| Message ID | 8dabce11b6bc9dc4ba534a2d5cf169ca3d0a812d.1607449636.git.rgoldwyn@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix direct write with respect to inode locking | expand |
On 8.12.20 г. 20:42 ч., Goldwyn Rodrigues wrote: > From: Goldwyn Rodrigues <rgoldwyn@suse.com> > > Code Cleanup. > > Fold generic_write_checks() in btrfs_write_check(), because > generic_write_checks() is called before btrfs_write_check() in both > cases. The prototype of btrfs_write_check() has been changed to return > ssize_t and it can return zero as a valid error code. btrfs_write_check > now returns the count of I/O to be performed. > > Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com> Codewise LGTM, just one minor nit below, I guess David can fix it up during merge. with it addressed you can add my: Reviewed-by: Nikolay Borisov <nborisov@suse.com> > --- > fs/btrfs/file.c | 33 +++++++++++++++++---------------- > 1 file changed, 17 insertions(+), 16 deletions(-) > > diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c > index 0e41459b8de6..272660a8279f 100644 > --- a/fs/btrfs/file.c > +++ b/fs/btrfs/file.c > @@ -1583,17 +1583,28 @@ static void update_time_for_write(struct inode *inode) > inode_inc_iversion(inode); > } > > -static int btrfs_write_check(struct kiocb *iocb, struct iov_iter *from, > - size_t count) > +/* btrfs_write_check - checks if a write can be performed > + * > + * Returns: > + * count - in case the write can be successfully performed nit: count - in case the write can be successfully performed number of bytes to write <snip>
On Tue, Dec 08, 2020 at 12:42:40PM -0600, Goldwyn Rodrigues wrote: > From: Goldwyn Rodrigues <rgoldwyn@suse.com> > > Code Cleanup. > > Fold generic_write_checks() in btrfs_write_check(), because > generic_write_checks() is called before btrfs_write_check() in both > cases. The prototype of btrfs_write_check() has been changed to return > ssize_t and it can return zero as a valid error code. btrfs_write_check > now returns the count of I/O to be performed. That's effectively reverting what Omar sent as a fix to your initial patch: https://lore.kernel.org/linux-btrfs/b096cecce8277b30e1c7e26efd0450c0bc12ff31.1605723568.git.osandov@fb.com/ fixing a problem. Now you revert that to fix another problem, now with the lock added. I'd rather have one patch without this cleanup and given that this is technically fixing a regression in the new 5.11 code it'll go to post rc1 pull request.
On 12:47 10/12, David Sterba wrote: > On Tue, Dec 08, 2020 at 12:42:40PM -0600, Goldwyn Rodrigues wrote: > > From: Goldwyn Rodrigues <rgoldwyn@suse.com> > > > > Code Cleanup. > > > > Fold generic_write_checks() in btrfs_write_check(), because > > generic_write_checks() is called before btrfs_write_check() in both > > cases. The prototype of btrfs_write_check() has been changed to return > > ssize_t and it can return zero as a valid error code. btrfs_write_check > > now returns the count of I/O to be performed. > > That's effectively reverting what Omar sent as a fix to your initial > patch: > > https://lore.kernel.org/linux-btrfs/b096cecce8277b30e1c7e26efd0450c0bc12ff31.1605723568.git.osandov@fb.com/ > > fixing a problem. Now you revert that to fix another problem, now with > the lock added. I'd rather have one patch without this cleanup and given > that this is technically fixing a regression in the new 5.11 code it'll > go to post rc1 pull request. This patchset fixes the problems mentioned there since both count and pos are accessed after the btrfs_write_check is called. However, this should be rejected because of the RWF_ENCODED work. I will post a patch for fixing the regression only.
diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c index 0e41459b8de6..272660a8279f 100644 --- a/fs/btrfs/file.c +++ b/fs/btrfs/file.c @@ -1583,17 +1583,28 @@ static void update_time_for_write(struct inode *inode) inode_inc_iversion(inode); } -static int btrfs_write_check(struct kiocb *iocb, struct iov_iter *from, - size_t count) +/* btrfs_write_check - checks if a write can be performed + * + * Returns: + * count - in case the write can be successfully performed + * < 0 - error in case write cannot be performed + * 0 - if the write is not required + */ +static ssize_t btrfs_write_check(struct kiocb *iocb, struct iov_iter *from) { struct file *file = iocb->ki_filp; struct inode *inode = file_inode(file); struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb); loff_t pos = iocb->ki_pos; - int ret; + ssize_t ret; + ssize_t count; loff_t oldsize; loff_t start_pos; + count = generic_write_checks(iocb, from); + if (count <= 0) + return count; + if (iocb->ki_flags & IOCB_NOWAIT) { size_t nocow_bytes = count; @@ -1635,7 +1646,7 @@ static int btrfs_write_check(struct kiocb *iocb, struct iov_iter *from, } } - return 0; + return count; } static noinline ssize_t btrfs_buffered_write(struct kiocb *iocb, @@ -1665,14 +1676,10 @@ static noinline ssize_t btrfs_buffered_write(struct kiocb *iocb, if (ret < 0) return ret; - ret = generic_write_checks(iocb, i); + ret = btrfs_write_check(iocb, i); if (ret <= 0) goto out; - ret = btrfs_write_check(iocb, i, ret); - if (ret < 0) - goto out; - pos = iocb->ki_pos; nrptrs = min(DIV_ROUND_UP(iov_iter_count(i), PAGE_SIZE), PAGE_SIZE / (sizeof(struct page *))); @@ -1920,14 +1927,8 @@ static ssize_t btrfs_direct_write(struct kiocb *iocb, struct iov_iter *from) if (err < 0) return err; - err = generic_write_checks(iocb, from); + err = btrfs_write_check(iocb, from); if (err <= 0) { - btrfs_inode_unlock(inode, ilock_flags); - return err; - } - - err = btrfs_write_check(iocb, from, err); - if (err < 0) { btrfs_inode_unlock(inode, ilock_flags); goto out; }