| Message ID | 20240830-vfs-file-f_version-v1-18-6d3e4816aa7b@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | file: remove f_version | expand |
On Fri 30-08-24 15:04:59, Christian Brauner wrote: > Only regular files with FMODE_ATOMIC_POS and directories need > f_pos_lock. Place a new f_pipe member in a union with f_pos_lock > that they can use and make them stop abusing f_version in follow-up > patches. > > Signed-off-by: Christian Brauner <brauner@kernel.org> What makes me a bit uneasy is that we do mutex_init() on the space in struct file and then pipe_open() will clobber it. And then we eventually call mutex_destroy() on the clobbered mutex. I think so far this does no harm but mostly by luck. I think we need to make sure that when f_pos_lock is unused, we don't even call mutex_init() / mutex_destroy() on it. Honza > --- > include/linux/fs.h | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/include/linux/fs.h b/include/linux/fs.h > index 3e6b3c1afb31..ca4925008244 100644 > --- a/include/linux/fs.h > +++ b/include/linux/fs.h > @@ -1001,6 +1001,7 @@ static inline int ra_has_index(struct file_ra_state *ra, pgoff_t index) > * @f_cred: stashed credentials of creator/opener > * @f_path: path of the file > * @f_pos_lock: lock protecting file position > + * @f_pipe: specific to pipes > * @f_pos: file position > * @f_version: file version > * @f_security: LSM security context of this file > @@ -1026,7 +1027,12 @@ struct file { > const struct cred *f_cred; > /* --- cacheline 1 boundary (64 bytes) --- */ > struct path f_path; > - struct mutex f_pos_lock; > + union { > + /* regular files (with FMODE_ATOMIC_POS) and directories */ > + struct mutex f_pos_lock; > + /* pipes */ > + u64 f_pipe; > + }; > loff_t f_pos; > u64 f_version; > /* --- cacheline 2 boundary (128 bytes) --- */ > > -- > 2.45.2 >
On Tue, Sep 03, 2024 at 03:50:55PM GMT, Jan Kara wrote: > On Fri 30-08-24 15:04:59, Christian Brauner wrote: > > Only regular files with FMODE_ATOMIC_POS and directories need > > f_pos_lock. Place a new f_pipe member in a union with f_pos_lock > > that they can use and make them stop abusing f_version in follow-up > > patches. > > > > Signed-off-by: Christian Brauner <brauner@kernel.org> > > What makes me a bit uneasy is that we do mutex_init() on the space in > struct file and then pipe_open() will clobber it. And then we eventually > call mutex_destroy() on the clobbered mutex. I think so far this does no We don't call mutex_destroy() on it and don't need to. And calling mutex_init() is fine precisely because pipes do use it. It would be really ugly do ensure that mutex_init() isn't called for pipes. But I'll add a comment.
On Tue 03-09-24 16:31:28, Christian Brauner wrote: > On Tue, Sep 03, 2024 at 03:50:55PM GMT, Jan Kara wrote: > > On Fri 30-08-24 15:04:59, Christian Brauner wrote: > > > Only regular files with FMODE_ATOMIC_POS and directories need > > > f_pos_lock. Place a new f_pipe member in a union with f_pos_lock > > > that they can use and make them stop abusing f_version in follow-up > > > patches. > > > > > > Signed-off-by: Christian Brauner <brauner@kernel.org> > > > > What makes me a bit uneasy is that we do mutex_init() on the space in > > struct file and then pipe_open() will clobber it. And then we eventually > > call mutex_destroy() on the clobbered mutex. I think so far this does no > > We don't call mutex_destroy() on it and don't need to. Ah, right, we don't bother with that for f_pos_lock. > And calling mutex_init() is fine precisely because pipes do use it. It > would be really ugly do ensure that mutex_init() isn't called for pipes. > But I'll add a comment. I'm not sure I understand what do you mean by "calling mutex_init() is fine precisely because pipes do use it." Perhaps you meant "don't use it"? Otherwise after looking at it for a while I agree the cure would be likely worse than the disease so a comment is as good as it gets I guess. Honza
On Fri, Aug 30, 2024 at 03:04:59PM +0200, Christian Brauner wrote: > Only regular files with FMODE_ATOMIC_POS and directories need > f_pos_lock. Place a new f_pipe member in a union with f_pos_lock > that they can use and make them stop abusing f_version in follow-up > patches. Not sure I like that - having lseek(2) use a separate primitive instead of fdget_pos(), grabbing ->f_pos_lock for _everything_ that has FMODE_LSEEK, directory or no directory, would simplify quite a few things. OTOH, that will affect only the explanation of validity - pipes do *not* have FMODE_LSEEK, so it becomes "fdget_pos() and fdget_seek() are the only things that might want ->f_pos_lock, and neither touch it for pipes - fdget_pos() because FMODE_ATOMIC_POS is not there and fdget_seek() because FMODE_LSEEK isn't". Oh, well...
diff --git a/include/linux/fs.h b/include/linux/fs.h index 3e6b3c1afb31..ca4925008244 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -1001,6 +1001,7 @@ static inline int ra_has_index(struct file_ra_state *ra, pgoff_t index) * @f_cred: stashed credentials of creator/opener * @f_path: path of the file * @f_pos_lock: lock protecting file position + * @f_pipe: specific to pipes * @f_pos: file position * @f_version: file version * @f_security: LSM security context of this file @@ -1026,7 +1027,12 @@ struct file { const struct cred *f_cred; /* --- cacheline 1 boundary (64 bytes) --- */ struct path f_path; - struct mutex f_pos_lock; + union { + /* regular files (with FMODE_ATOMIC_POS) and directories */ + struct mutex f_pos_lock; + /* pipes */ + u64 f_pipe; + }; loff_t f_pos; u64 f_version; /* --- cacheline 2 boundary (128 bytes) --- */
Only regular files with FMODE_ATOMIC_POS and directories need f_pos_lock. Place a new f_pipe member in a union with f_pos_lock that they can use and make them stop abusing f_version in follow-up patches. Signed-off-by: Christian Brauner <brauner@kernel.org> --- include/linux/fs.h | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)