Message ID | 20220607150549.217390-1-jlayton@kernel.org (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | ceph: convert to generic_file_llseek | expand |
Jeff Layton <jlayton@kernel.org> writes: > There's no reason we need to lock the inode for write in order to handle > an llseek. I suspect this should have been dropped in 2013 when we > stopped doing vmtruncate in llseek. > > With that gone, ceph_llseek is functionally equivalent to > generic_file_llseek, so just call that after getting the size. > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > fs/ceph/file.c | 52 +++++--------------------------------------------- > 1 file changed, 5 insertions(+), 47 deletions(-) > Nice! I started reviewing your previous patch, and while checking other filesystems I wondered if the generic_* could be used instead. And here it is. And it may even fix races in the SEEK_CUR by locking f_lock. Reviewed-by: Luís Henriques <lhenriques@suse.de> Cheers,
On 6/7/22 11:05 PM, Jeff Layton wrote: > There's no reason we need to lock the inode for write in order to handle > an llseek. I suspect this should have been dropped in 2013 when we > stopped doing vmtruncate in llseek. > > With that gone, ceph_llseek is functionally equivalent to > generic_file_llseek, so just call that after getting the size. > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > fs/ceph/file.c | 52 +++++--------------------------------------------- > 1 file changed, 5 insertions(+), 47 deletions(-) > > diff --git a/fs/ceph/file.c b/fs/ceph/file.c > index 0c13a3f23c99..0e82a1c383ca 100644 > --- a/fs/ceph/file.c > +++ b/fs/ceph/file.c > @@ -1989,57 +1989,15 @@ static ssize_t ceph_write_iter(struct kiocb *iocb, struct iov_iter *from) > */ > static loff_t ceph_llseek(struct file *file, loff_t offset, int whence) > { > - struct inode *inode = file->f_mapping->host; > - struct ceph_fs_client *fsc = ceph_inode_to_client(inode); > - loff_t i_size; > - loff_t ret; > - > - inode_lock(inode); > - > if (whence == SEEK_END || whence == SEEK_DATA || whence == SEEK_HOLE) { > + struct inode *inode = file_inode(file); > + int ret; > + > ret = ceph_do_getattr(inode, CEPH_STAT_CAP_SIZE, false); > if (ret < 0) > - goto out; > - } > - > - i_size = i_size_read(inode); > - switch (whence) { > - case SEEK_END: > - offset += i_size; > - break; > - case SEEK_CUR: > - /* > - * Here we special-case the lseek(fd, 0, SEEK_CUR) > - * position-querying operation. Avoid rewriting the "same" > - * f_pos value back to the file because a concurrent read(), > - * write() or lseek() might have altered it > - */ > - if (offset == 0) { > - ret = file->f_pos; > - goto out; > - } > - offset += file->f_pos; > - break; > - case SEEK_DATA: > - if (offset < 0 || offset >= i_size) { > - ret = -ENXIO; > - goto out; > - } > - break; > - case SEEK_HOLE: > - if (offset < 0 || offset >= i_size) { > - ret = -ENXIO; > - goto out; > - } > - offset = i_size; > - break; > + return ret; > } > - > - ret = vfs_setpos(file, offset, max(i_size, fsc->max_file_size)); > - > -out: > - inode_unlock(inode); > - return ret; > + return generic_file_llseek(file, offset, whence); > } > > static inline void ceph_zero_partial_page( LGTM. Merged into the testing branch. Thanks Jeff. -- Xiubo
diff --git a/fs/ceph/file.c b/fs/ceph/file.c index 0c13a3f23c99..0e82a1c383ca 100644 --- a/fs/ceph/file.c +++ b/fs/ceph/file.c @@ -1989,57 +1989,15 @@ static ssize_t ceph_write_iter(struct kiocb *iocb, struct iov_iter *from) */ static loff_t ceph_llseek(struct file *file, loff_t offset, int whence) { - struct inode *inode = file->f_mapping->host; - struct ceph_fs_client *fsc = ceph_inode_to_client(inode); - loff_t i_size; - loff_t ret; - - inode_lock(inode); - if (whence == SEEK_END || whence == SEEK_DATA || whence == SEEK_HOLE) { + struct inode *inode = file_inode(file); + int ret; + ret = ceph_do_getattr(inode, CEPH_STAT_CAP_SIZE, false); if (ret < 0) - goto out; - } - - i_size = i_size_read(inode); - switch (whence) { - case SEEK_END: - offset += i_size; - break; - case SEEK_CUR: - /* - * Here we special-case the lseek(fd, 0, SEEK_CUR) - * position-querying operation. Avoid rewriting the "same" - * f_pos value back to the file because a concurrent read(), - * write() or lseek() might have altered it - */ - if (offset == 0) { - ret = file->f_pos; - goto out; - } - offset += file->f_pos; - break; - case SEEK_DATA: - if (offset < 0 || offset >= i_size) { - ret = -ENXIO; - goto out; - } - break; - case SEEK_HOLE: - if (offset < 0 || offset >= i_size) { - ret = -ENXIO; - goto out; - } - offset = i_size; - break; + return ret; } - - ret = vfs_setpos(file, offset, max(i_size, fsc->max_file_size)); - -out: - inode_unlock(inode); - return ret; + return generic_file_llseek(file, offset, whence); } static inline void ceph_zero_partial_page(
There's no reason we need to lock the inode for write in order to handle an llseek. I suspect this should have been dropped in 2013 when we stopped doing vmtruncate in llseek. With that gone, ceph_llseek is functionally equivalent to generic_file_llseek, so just call that after getting the size. Signed-off-by: Jeff Layton <jlayton@kernel.org> --- fs/ceph/file.c | 52 +++++--------------------------------------------- 1 file changed, 5 insertions(+), 47 deletions(-)