* [PATCH v2] btrfs: add goto in btrfs_defrag_file for error handling @ 2021-05-05 1:26 Tian Tao 2021-05-05 22:40 ` Boris Burkov 0 siblings, 1 reply; 3+ messages in thread From: Tian Tao @ 2021-05-05 1:26 UTC (permalink / raw) To: dsterba, josef, clm; +Cc: linux-btrfs, Tian Tao ret is assigned -EAGAIN at line 1455 and then reassigned defrag_count at line 1547 after exiting the while loop.this causes the btrfs_defrag_file function to not return the correct value in the event of an error, this patch fixed this issue. Signed-off-by: Tian Tao <tiantao6@hisilicon.com> --- v2: rewrite the patch, patch name and commit message. --- fs/btrfs/ioctl.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index ee1dbab..5713450 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -1453,7 +1453,7 @@ int btrfs_defrag_file(struct inode *inode, struct file *file, if (btrfs_defrag_cancelled(fs_info)) { btrfs_debug(fs_info, "defrag_file cancelled"); ret = -EAGAIN; - break; + goto error; } if (!should_defrag_range(inode, (u64)i << PAGE_SHIFT, @@ -1531,6 +1531,8 @@ int btrfs_defrag_file(struct inode *inode, struct file *file, } } + ret = defrag_count; +error: if ((range->flags & BTRFS_DEFRAG_RANGE_START_IO)) { filemap_flush(inode->i_mapping); if (test_bit(BTRFS_INODE_HAS_ASYNC_EXTENT, @@ -1544,8 +1546,6 @@ int btrfs_defrag_file(struct inode *inode, struct file *file, btrfs_set_fs_incompat(fs_info, COMPRESS_ZSTD); } - ret = defrag_count; - out_ra: if (do_compress) { btrfs_inode_lock(inode, 0); -- 2.7.4 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2] btrfs: add goto in btrfs_defrag_file for error handling 2021-05-05 1:26 [PATCH v2] btrfs: add goto in btrfs_defrag_file for error handling Tian Tao @ 2021-05-05 22:40 ` Boris Burkov 2021-05-17 13:00 ` David Sterba 0 siblings, 1 reply; 3+ messages in thread From: Boris Burkov @ 2021-05-05 22:40 UTC (permalink / raw) To: Tian Tao; +Cc: dsterba, josef, clm, linux-btrfs On Wed, May 05, 2021 at 09:26:28AM +0800, Tian Tao wrote: > ret is assigned -EAGAIN at line 1455 and then reassigned defrag_count > at line 1547 after exiting the while loop.this causes the > btrfs_defrag_file function to not return the correct value in the event > of an error, this patch fixed this issue. This looks like a correct fix, in that it locally improves what it claims to improve. However, I have some questions about the style and consistency of the function as a whole as a result. I think Dave had a similar comment in his very first reply on v1. The loop has the following early exit points: fs unmounted cancellation swapfile/error in cluster_pages_for_defrag newer_off == (u64)-1 error (ENOMEM or ENOENT) in find_new_extents To me, it is confusing that of all these, only cancellation goes to a label called "error". I would expect at least the swapfile/cluster case to also jump to error. find_new_extents is interesting, because ENOENT could be semantically special as an error and warrant a break rather than a goto error, so we ought to figure that out correctly too. If there is some good reason that only cancellation should receive this treatment, and that some early exit cases should break or goto out_ra then I would at least name the new label "cancel" and write a comment or a note in the git commit explaining the difference. Thinking out loud, I suspect a way to really fix this messy function is to do something like lift the contents of the while loop into a helper function which returns the next incremental defrag_count, an error, or 0 for done. Thanks for the fix, Boris > > Signed-off-by: Tian Tao <tiantao6@hisilicon.com> > --- > > v2: rewrite the patch, patch name and commit message. > --- > fs/btrfs/ioctl.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > index ee1dbab..5713450 100644 > --- a/fs/btrfs/ioctl.c > +++ b/fs/btrfs/ioctl.c > @@ -1453,7 +1453,7 @@ int btrfs_defrag_file(struct inode *inode, struct file *file, > if (btrfs_defrag_cancelled(fs_info)) { > btrfs_debug(fs_info, "defrag_file cancelled"); > ret = -EAGAIN; > - break; > + goto error; > } > > if (!should_defrag_range(inode, (u64)i << PAGE_SHIFT, > @@ -1531,6 +1531,8 @@ int btrfs_defrag_file(struct inode *inode, struct file *file, > } > } > > + ret = defrag_count; > +error: > if ((range->flags & BTRFS_DEFRAG_RANGE_START_IO)) { > filemap_flush(inode->i_mapping); > if (test_bit(BTRFS_INODE_HAS_ASYNC_EXTENT, > @@ -1544,8 +1546,6 @@ int btrfs_defrag_file(struct inode *inode, struct file *file, > btrfs_set_fs_incompat(fs_info, COMPRESS_ZSTD); > } > > - ret = defrag_count; > - > out_ra: > if (do_compress) { > btrfs_inode_lock(inode, 0); > -- > 2.7.4 > ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2] btrfs: add goto in btrfs_defrag_file for error handling 2021-05-05 22:40 ` Boris Burkov @ 2021-05-17 13:00 ` David Sterba 0 siblings, 0 replies; 3+ messages in thread From: David Sterba @ 2021-05-17 13:00 UTC (permalink / raw) To: Boris Burkov; +Cc: Tian Tao, dsterba, josef, clm, linux-btrfs On Wed, May 05, 2021 at 03:40:52PM -0700, Boris Burkov wrote: > On Wed, May 05, 2021 at 09:26:28AM +0800, Tian Tao wrote: > > ret is assigned -EAGAIN at line 1455 and then reassigned defrag_count > > at line 1547 after exiting the while loop.this causes the > > btrfs_defrag_file function to not return the correct value in the event > > of an error, this patch fixed this issue. > > This looks like a correct fix, in that it locally improves what it > claims to improve. However, I have some questions about the style and > consistency of the function as a whole as a result. I think Dave had > a similar comment in his very first reply on v1. > > The loop has the following early exit points: > fs unmounted > cancellation > swapfile/error in cluster_pages_for_defrag > newer_off == (u64)-1 > error (ENOMEM or ENOENT) in find_new_extents > > To me, it is confusing that of all these, only cancellation goes to a > label called "error". I would expect at least the swapfile/cluster case > to also jump to error. find_new_extents is interesting, because ENOENT > could be semantically special as an error and warrant a break rather > than a goto error, so we ought to figure that out correctly too. > > If there is some good reason that only cancellation should receive this > treatment, and that some early exit cases should break or goto out_ra > then I would at least name the new label "cancel" and write a comment or > a note in the git commit explaining the difference. The naming convention of the exit labels describes what happens at the label point and not the reason, as the label can be targeted from various branches but the same clanup is done. The naming is not consistent everywhere, but at least that's the idea. > Thinking out loud, I suspect a way to really fix this messy function is > to do something like lift the contents of the while loop into a helper > function which returns the next incremental defrag_count, an error, or 0 > for done. Reading it again with the above in mind, there are two types of errors to end the defrag: - if some defrag work has been done but not entire file was processed - the rest, eg. some hard errors In the first case the optional flushing should still happen. In both cases the incompat bits should be set -- this is now missing. I'm not sure if the whole while loop could be factored out, there's a lot of shared state with the function. The different kinds of errors would have to be reflected too but that's doable. As this patch fixes the return value of canceled defrag, I'd take it as is and address the other issues separately. ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-05-17 13:03 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-05-05 1:26 [PATCH v2] btrfs: add goto in btrfs_defrag_file for error handling Tian Tao 2021-05-05 22:40 ` Boris Burkov 2021-05-17 13:00 ` David Sterba
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.