| Message ID | 20240131205237.3540210-4-bvanassche@acm.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Restore data lifetime support | expand |
On Wed, Jan 31, 2024 at 12:52:34PM -0800, Bart Van Assche wrote: > Split fcntl_rw_hint() such that there is one helper function per fcntl. No > functionality is changed by this patch. > > Reviewed-by: Christoph Hellwig <hch@lst.de> > Suggested-by: Christoph Hellwig <hch@lst.de> > Reviewed-by: Kanchan Joshi <joshi.k@samsung.com> > Cc: Jeff Layton <jlayton@kernel.org> > Cc: Chuck Lever <chuck.lever@oracle.com> > Cc: Jens Axboe <axboe@kernel.dk> > Cc: Stephen Rothwell <sfr@canb.auug.org.au> > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- > fs/fcntl.c | 47 ++++++++++++++++++++++++++--------------------- > 1 file changed, 26 insertions(+), 21 deletions(-) > > diff --git a/fs/fcntl.c b/fs/fcntl.c > index f3bc4662455f..5fa2d95114bf 100644 > --- a/fs/fcntl.c > +++ b/fs/fcntl.c > @@ -290,32 +290,35 @@ static bool rw_hint_valid(u64 hint) > } > } > > -static long fcntl_rw_hint(struct file *file, unsigned int cmd, > - unsigned long arg) > +static long fcntl_get_rw_hint(struct file *file, unsigned int cmd, > + unsigned long arg) > { > struct inode *inode = file_inode(file); > u64 __user *argp = (u64 __user *)arg; > - u64 hint; > + u64 hint = inode->i_write_hint; > > - switch (cmd) { > - case F_GET_RW_HINT: > - hint = inode->i_write_hint; > - if (copy_to_user(argp, &hint, sizeof(*argp))) > - return -EFAULT; > - return 0; > - case F_SET_RW_HINT: > - if (copy_from_user(&hint, argp, sizeof(hint))) > - return -EFAULT; > - if (!rw_hint_valid(hint)) > - return -EINVAL; > + if (copy_to_user(argp, &hint, sizeof(*argp))) > + return -EFAULT; > + return 0; > +} > > - inode_lock(inode); > - inode->i_write_hint = hint; > - inode_unlock(inode); > - return 0; > - default: > +static long fcntl_set_rw_hint(struct file *file, unsigned int cmd, > + unsigned long arg) > +{ > + struct inode *inode = file_inode(file); > + u64 __user *argp = (u64 __user *)arg; > + u64 hint; > + > + if (copy_from_user(&hint, argp, sizeof(hint))) > + return -EFAULT; > + if (!rw_hint_valid(hint)) > return -EINVAL; > - } > + > + inode_lock(inode); > + inode->i_write_hint = hint; > + inode_unlock(inode); What is this locking serialising against? The inode may or may not be locked when we access this in IO path, so why isn't this just WRITE_ONCE() here and READ_ONCE() in the IO paths? -Dave.
On 1/31/24 13:07, Dave Chinner wrote: > On Wed, Jan 31, 2024 at 12:52:34PM -0800, Bart Van Assche wrote: >> + inode_lock(inode); >> + inode->i_write_hint = hint; >> + inode_unlock(inode); > > What is this locking serialising against? The inode may or may not > be locked when we access this in IO path, so why isn't this just > WRITE_ONCE() here and READ_ONCE() in the IO paths? How about using WRITE_ONCE()/READ_ONCE() in the fcntl implementations and regular reads in the I/O paths? Using F_SET_RW_HINT while I/O is ongoing is racy - there are no guarantees about how F_SET_RW_HINT will affect I/O that has already been submitted. Hence, I think that it is acceptable to use regular reads for i_write_hint in the I/O paths. Thanks, Bart.
diff --git a/fs/fcntl.c b/fs/fcntl.c index f3bc4662455f..5fa2d95114bf 100644 --- a/fs/fcntl.c +++ b/fs/fcntl.c @@ -290,32 +290,35 @@ static bool rw_hint_valid(u64 hint) } } -static long fcntl_rw_hint(struct file *file, unsigned int cmd, - unsigned long arg) +static long fcntl_get_rw_hint(struct file *file, unsigned int cmd, + unsigned long arg) { struct inode *inode = file_inode(file); u64 __user *argp = (u64 __user *)arg; - u64 hint; + u64 hint = inode->i_write_hint; - switch (cmd) { - case F_GET_RW_HINT: - hint = inode->i_write_hint; - if (copy_to_user(argp, &hint, sizeof(*argp))) - return -EFAULT; - return 0; - case F_SET_RW_HINT: - if (copy_from_user(&hint, argp, sizeof(hint))) - return -EFAULT; - if (!rw_hint_valid(hint)) - return -EINVAL; + if (copy_to_user(argp, &hint, sizeof(*argp))) + return -EFAULT; + return 0; +} - inode_lock(inode); - inode->i_write_hint = hint; - inode_unlock(inode); - return 0; - default: +static long fcntl_set_rw_hint(struct file *file, unsigned int cmd, + unsigned long arg) +{ + struct inode *inode = file_inode(file); + u64 __user *argp = (u64 __user *)arg; + u64 hint; + + if (copy_from_user(&hint, argp, sizeof(hint))) + return -EFAULT; + if (!rw_hint_valid(hint)) return -EINVAL; - } + + inode_lock(inode); + inode->i_write_hint = hint; + inode_unlock(inode); + + return 0; } static long do_fcntl(int fd, unsigned int cmd, unsigned long arg, @@ -421,8 +424,10 @@ static long do_fcntl(int fd, unsigned int cmd, unsigned long arg, err = memfd_fcntl(filp, cmd, argi); break; case F_GET_RW_HINT: + err = fcntl_get_rw_hint(filp, cmd, arg); + break; case F_SET_RW_HINT: - err = fcntl_rw_hint(filp, cmd, arg); + err = fcntl_set_rw_hint(filp, cmd, arg); break; default: break;