| Message ID | 1515603334-21423-1-git-send-email-josef@toxicpanda.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2018年01月11日 00:55, Josef Bacik wrote: > From: Josef Bacik <jbacik@fb.com> > > My delayed refs rsv patches uncovered a problem in > btrfs_alloc_data_chunk_ondemand where we don't clear ret before > returning, so we could have whatever left over value we had from trying > to do a chunk allocation or whatever that may have failed. Since we > know we've succeeded at this point just unconditionally return 0. This > fixed the xfstests failures I was seeing with my delayed refs rsv > patches. > > Signed-off-by: Josef Bacik <jbacik@fb.com> Reviewed-by: Qu Wenruo <wqu@suse.com> The branch to leave ret = 1 would be the "ret = do_chunk_alloc()" call, which could return 1, and goes to final return. So the fix is good. Thanks, Qu > --- > fs/btrfs/extent-tree.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c > index dbae25d882de..33c9efbfc9a7 100644 > --- a/fs/btrfs/extent-tree.c > +++ b/fs/btrfs/extent-tree.c > @@ -4387,8 +4387,7 @@ int btrfs_alloc_data_chunk_ondemand(struct btrfs_inode *inode, u64 bytes) > trace_btrfs_space_reservation(fs_info, "space_info", > data_sinfo->flags, bytes, 1); > spin_unlock(&data_sinfo->lock); > - > - return ret; > + return 0; > } > > int btrfs_check_data_free_space(struct inode *inode, >
On 10.01.2018 18:55, Josef Bacik wrote: > From: Josef Bacik <jbacik@fb.com> > > My delayed refs rsv patches uncovered a problem in > btrfs_alloc_data_chunk_ondemand where we don't clear ret before > returning, so we could have whatever left over value we had from trying > to do a chunk allocation or whatever that may have failed. Since we > know we've succeeded at this point just unconditionally return 0. This > fixed the xfstests failures I was seeing with my delayed refs rsv > patches. > > Signed-off-by: Josef Bacik <jbacik@fb.com> > --- > fs/btrfs/extent-tree.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c > index dbae25d882de..33c9efbfc9a7 100644 > --- a/fs/btrfs/extent-tree.c > +++ b/fs/btrfs/extent-tree.c > @@ -4387,8 +4387,7 @@ int btrfs_alloc_data_chunk_ondemand(struct btrfs_inode *inode, u64 bytes) > trace_btrfs_space_reservation(fs_info, "space_info", > data_sinfo->flags, bytes, 1); > spin_unlock(&data_sinfo->lock); > - > - return ret; > + return 0; > } > While this patch indeed seems to fix some failure I was seeing with non-empty delalloc resv I fail to see why returning 1 could cause them. In btrfs_check_data_free_space we only check if ret is < 0 and after that we call btrfs_qgroup_reserve_data so this can't cause an issue. Then we have usage in btrfs_zero_range which is a new code and is not really used-per se in the tests that I saw failing. So the last remaining possible culprit is the usage in btrfs_fallocate (and indeed adding printk's shows that everytime a test fail we indeed returned 1 from alloc_data_chunk. However, looking at the code in fallocate ret is always overwritten before being checked. So how exactly is returning a positive value influencing the freeing of per-inode block rsvs? > int btrfs_check_data_free_space(struct inode *inode, > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 10, 2018 at 11:55:34AM -0500, Josef Bacik wrote: > From: Josef Bacik <jbacik@fb.com> > > My delayed refs rsv patches uncovered a problem in > btrfs_alloc_data_chunk_ondemand where we don't clear ret before > returning, so we could have whatever left over value we had from trying > to do a chunk allocation or whatever that may have failed. Since we > know we've succeeded at this point just unconditionally return 0. This > fixed the xfstests failures I was seeing with my delayed refs rsv > patches. > > Signed-off-by: Josef Bacik <jbacik@fb.com> > --- > fs/btrfs/extent-tree.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c > index dbae25d882de..33c9efbfc9a7 100644 > --- a/fs/btrfs/extent-tree.c > +++ b/fs/btrfs/extent-tree.c > @@ -4387,8 +4387,7 @@ int btrfs_alloc_data_chunk_ondemand(struct btrfs_inode *inode, u64 bytes) > trace_btrfs_space_reservation(fs_info, "space_info", > data_sinfo->flags, bytes, 1); > spin_unlock(&data_sinfo->lock); > - > - return ret; > + return 0; I don't think this is the right way to fix it. The return code of do_chunk_alloc depends on the force parameter, and in case it's CHUNK_ALLOC_NO_FORCE the caller should handle all the possibilities, ie. negative/0/positive. Other callers do that, so I'd rather see it fixed right after do_chunk_alloc and not forcing 0. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c index dbae25d882de..33c9efbfc9a7 100644 --- a/fs/btrfs/extent-tree.c +++ b/fs/btrfs/extent-tree.c @@ -4387,8 +4387,7 @@ int btrfs_alloc_data_chunk_ondemand(struct btrfs_inode *inode, u64 bytes) trace_btrfs_space_reservation(fs_info, "space_info", data_sinfo->flags, bytes, 1); spin_unlock(&data_sinfo->lock); - - return ret; + return 0; } int btrfs_check_data_free_space(struct inode *inode,