| Message ID | 20180928111821.24376-8-josef@toxicpanda.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | My current patch queue | expand |
On Fri, Sep 28, 2018 at 07:17:46AM -0400, Josef Bacik wrote: > may_commit_transaction will skip committing the transaction if we don't > have enough pinned space or if we're trying to find space for a SYSTEM > chunk. However if we have pending free block groups in this transaction > we still want to commit as we may be able to allocate a chunk to make > our reservation. So instead of just returning ENOSPC, check if we have > free block groups pending, and if so commit the transaction to allow us > to use that free space. > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> > Reviewed-by: Omar Sandoval <osandov@fb.com> > --- > fs/btrfs/extent-tree.c | 33 +++++++++++++++++++-------------- > 1 file changed, 19 insertions(+), 14 deletions(-) > > diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c > index 1213f573eea2..da73b3e5bc39 100644 > --- a/fs/btrfs/extent-tree.c > +++ b/fs/btrfs/extent-tree.c > @@ -4830,10 +4830,18 @@ static int may_commit_transaction(struct btrfs_fs_info *fs_info, > if (!bytes) > return 0; > > - /* See if there is enough pinned space to make this reservation */ > - if (__percpu_counter_compare(&space_info->total_bytes_pinned, > - bytes, > - BTRFS_TOTAL_BYTES_PINNED_BATCH) >= 0) > + trans = btrfs_join_transaction(fs_info->extent_root); > + if (IS_ERR(trans)) > + return -ENOSPC; Why do you set override the error to ENOSPC, instead of passing PTR_ERR(trans)? There are more reasons why btrfs_join_transaction could fail: EROFS after error, EDQUOT when quotas say no, ENOMEM from ulist_add, that's just a sample I quickly found.
On Thu, Oct 04, 2018 at 01:24:24PM +0200, David Sterba wrote: > On Fri, Sep 28, 2018 at 07:17:46AM -0400, Josef Bacik wrote: > > may_commit_transaction will skip committing the transaction if we don't > > have enough pinned space or if we're trying to find space for a SYSTEM > > chunk. However if we have pending free block groups in this transaction > > we still want to commit as we may be able to allocate a chunk to make > > our reservation. So instead of just returning ENOSPC, check if we have > > free block groups pending, and if so commit the transaction to allow us > > to use that free space. > > > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> > > Reviewed-by: Omar Sandoval <osandov@fb.com> > > --- > > fs/btrfs/extent-tree.c | 33 +++++++++++++++++++-------------- > > 1 file changed, 19 insertions(+), 14 deletions(-) > > > > diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c > > index 1213f573eea2..da73b3e5bc39 100644 > > --- a/fs/btrfs/extent-tree.c > > +++ b/fs/btrfs/extent-tree.c > > @@ -4830,10 +4830,18 @@ static int may_commit_transaction(struct btrfs_fs_info *fs_info, > > if (!bytes) > > return 0; > > > > - /* See if there is enough pinned space to make this reservation */ > > - if (__percpu_counter_compare(&space_info->total_bytes_pinned, > > - bytes, > > - BTRFS_TOTAL_BYTES_PINNED_BATCH) >= 0) > > + trans = btrfs_join_transaction(fs_info->extent_root); > > + if (IS_ERR(trans)) > > + return -ENOSPC; > > Why do you set override the error to ENOSPC, instead of passing > PTR_ERR(trans)? There are more reasons why btrfs_join_transaction could > fail: EROFS after error, EDQUOT when quotas say no, ENOMEM from > ulist_add, that's just a sample I quickly found. (argh I hit r instead of g, sending again) Because may_commit_transaction is only called during flushing, we don't actually care about the value, just that it failed. Thanks, Josef
On Thu, Oct 11, 2018 at 02:33:55PM -0400, Josef Bacik wrote: > On Thu, Oct 04, 2018 at 01:24:24PM +0200, David Sterba wrote: > > On Fri, Sep 28, 2018 at 07:17:46AM -0400, Josef Bacik wrote: > > > may_commit_transaction will skip committing the transaction if we don't > > > have enough pinned space or if we're trying to find space for a SYSTEM > > > chunk. However if we have pending free block groups in this transaction > > > we still want to commit as we may be able to allocate a chunk to make > > > our reservation. So instead of just returning ENOSPC, check if we have > > > free block groups pending, and if so commit the transaction to allow us > > > to use that free space. > > > > > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> > > > Reviewed-by: Omar Sandoval <osandov@fb.com> > > > --- > > > fs/btrfs/extent-tree.c | 33 +++++++++++++++++++-------------- > > > 1 file changed, 19 insertions(+), 14 deletions(-) > > > > > > diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c > > > index 1213f573eea2..da73b3e5bc39 100644 > > > --- a/fs/btrfs/extent-tree.c > > > +++ b/fs/btrfs/extent-tree.c > > > @@ -4830,10 +4830,18 @@ static int may_commit_transaction(struct btrfs_fs_info *fs_info, > > > if (!bytes) > > > return 0; > > > > > > - /* See if there is enough pinned space to make this reservation */ > > > - if (__percpu_counter_compare(&space_info->total_bytes_pinned, > > > - bytes, > > > - BTRFS_TOTAL_BYTES_PINNED_BATCH) >= 0) > > > + trans = btrfs_join_transaction(fs_info->extent_root); > > > + if (IS_ERR(trans)) > > > + return -ENOSPC; > > > > Why do you set override the error to ENOSPC, instead of passing > > PTR_ERR(trans)? There are more reasons why btrfs_join_transaction could > > fail: EROFS after error, EDQUOT when quotas say no, ENOMEM from > > ulist_add, that's just a sample I quickly found. > > (argh I hit r instead of g, sending again) > > Because may_commit_transaction is only called during flushing, we don't actually > care about the value, just that it failed. Thanks, No, we do care about error values. If anything, here it's used in the tracepoint as the reason why the flushing failed. Conflating them into one without a good reason is a bad practice I don't want to let spread in the code.
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c index 1213f573eea2..da73b3e5bc39 100644 --- a/fs/btrfs/extent-tree.c +++ b/fs/btrfs/extent-tree.c @@ -4830,10 +4830,18 @@ static int may_commit_transaction(struct btrfs_fs_info *fs_info, if (!bytes) return 0; - /* See if there is enough pinned space to make this reservation */ - if (__percpu_counter_compare(&space_info->total_bytes_pinned, - bytes, - BTRFS_TOTAL_BYTES_PINNED_BATCH) >= 0) + trans = btrfs_join_transaction(fs_info->extent_root); + if (IS_ERR(trans)) + return -ENOSPC; + + /* + * See if there is enough pinned space to make this reservation, or if + * we have bg's that are going to be freed, allowing us to possibly do a + * chunk allocation the next loop through. + */ + if (test_bit(BTRFS_TRANS_HAVE_FREE_BGS, &trans->transaction->flags) || + __percpu_counter_compare(&space_info->total_bytes_pinned, bytes, + BTRFS_TOTAL_BYTES_PINNED_BATCH) >= 0) goto commit; /* @@ -4841,7 +4849,7 @@ static int may_commit_transaction(struct btrfs_fs_info *fs_info, * this reservation. */ if (space_info != delayed_rsv->space_info) - return -ENOSPC; + goto enospc; spin_lock(&delayed_rsv->lock); reclaim_bytes += delayed_rsv->reserved; @@ -4855,17 +4863,14 @@ static int may_commit_transaction(struct btrfs_fs_info *fs_info, bytes -= reclaim_bytes; if (__percpu_counter_compare(&space_info->total_bytes_pinned, - bytes, - BTRFS_TOTAL_BYTES_PINNED_BATCH) < 0) { - return -ENOSPC; - } - + bytes, + BTRFS_TOTAL_BYTES_PINNED_BATCH) < 0) + goto enospc; commit: - trans = btrfs_join_transaction(fs_info->extent_root); - if (IS_ERR(trans)) - return -ENOSPC; - return btrfs_commit_transaction(trans); +enospc: + btrfs_end_transaction(trans); + return -ENOSPC; } /*