On Wed, Oct 23, 2019 at 9:52 AM Qu Wenruo <[email protected]> wrote: > > [BUG] > When deleting large files (which cross block group boundary) with discard > mount option, we find some btrfs_discard_extent() calls only trimmed part > of its space, not the whole range: > > btrfs_discard_extent: type=0x1 start=19626196992 len=2144530432 > trimmed=1073741824 ratio=50% > > type: bbio->map_type, in above case, it's SINGLE DATA. > start: Logical address of this trim > len: Logical length of this trim > trimmed: Physically trimmed bytes > ratio: trimmed / len > > Thus leading some unused space not discarded. > > [CAUSE] > When discard mount option is specified, after a transaction is fully > committed (super block written to disk), we begin to cleanup pinned > extents in the following call chain: > btrfs_commit_transaction() > |- write_all_supers() > |- btrfs_finish_extent_commit() > |- find_first_extent_bit(unpin, 0, &start, &end, EXTENT_DIRTY); > |- btrfs_discard_extent() > > However pinned extents are recorded in an extent_io_tree, which can > merge same extent states.
same -> adjacent 'same' would imply duplicate entries (same start offset and lengths) > > When a large file get deleted and it has adjacent file extents across > block group boundary, we will get a large merged range. > > Then when we pass the large range into btrfs_discard_extent(), > btrfs_discard_extent() will just trim the first part, without trimming > the remaining part. > > Furthermore, this bug is not that reliably observed, as if the whole > block group is empty, there will be another trim for that block group. > > So the most obvious way to find this missing trim needs to delete large > extents at block group boundary without empting involved block groups. > > [FIX] > Fix this bug by calling btrfs_map_block() in a loop, until we reach the > end location. > > Signed-off-by: Qu Wenruo <[email protected]> With that small change: Reviewed-by: Filipe Manana <[email protected]> Thanks. > --- > fs/btrfs/extent-tree.c | 40 ++++++++++++++++++++++++++++++---------- > 1 file changed, 30 insertions(+), 10 deletions(-) > > diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c > index 49cb26fa7c63..45df45fa775b 100644 > --- a/fs/btrfs/extent-tree.c > +++ b/fs/btrfs/extent-tree.c > @@ -1306,8 +1306,10 @@ static int btrfs_issue_discard(struct block_device > *bdev, u64 start, u64 len, > int btrfs_discard_extent(struct btrfs_fs_info *fs_info, u64 bytenr, > u64 num_bytes, u64 *actual_bytes) > { > - int ret; > + int ret = 0; > u64 discarded_bytes = 0; > + u64 end = bytenr + num_bytes; > + u64 cur = bytenr; > struct btrfs_bio *bbio = NULL; > > > @@ -1316,15 +1318,22 @@ int btrfs_discard_extent(struct btrfs_fs_info > *fs_info, u64 bytenr, > * associated to its stripes that don't go away while we are > discarding. > */ > btrfs_bio_counter_inc_blocked(fs_info); > - /* Tell the block device(s) that the sectors can be discarded */ > - ret = btrfs_map_block(fs_info, BTRFS_MAP_DISCARD, bytenr, &num_bytes, > - &bbio, 0); > - /* Error condition is -ENOMEM */ > - if (!ret) { > - struct btrfs_bio_stripe *stripe = bbio->stripes; > + while (cur < end) { > + struct btrfs_bio_stripe *stripe; > int i; > > + /* Tell the block device(s) that the sectors can be discarded > */ > + ret = btrfs_map_block(fs_info, BTRFS_MAP_DISCARD, cur, > + &num_bytes, &bbio, 0); > + /* > + * Error can be -ENOMEM, -ENOENT (no such chunk mapping) or > + * -EOPNOTSUPP. For any such error, @num_bytes is not updated, > + * thus we can't continue anyway. > + */ > + if (ret < 0) > + goto out; > > + stripe = bbio->stripes; > for (i = 0; i < bbio->num_stripes; i++, stripe++) { > u64 bytes; > struct request_queue *req_q; > @@ -1341,10 +1350,19 @@ int btrfs_discard_extent(struct btrfs_fs_info > *fs_info, u64 bytenr, > stripe->physical, > stripe->length, > &bytes); > - if (!ret) > + if (!ret) { > discarded_bytes += bytes; > - else if (ret != -EOPNOTSUPP) > - break; /* Logic errors or -ENOMEM, or -EIO > but I don't know how that could happen JDM */ > + } else if (ret != -EOPNOTSUPP) { > + /* > + * Logic errors or -ENOMEM, or -EIO but I > don't > + * know how that could happen JDM > + * > + * Ans since there are two loops, explicitly > + * goto out to avoid confusion. > + */ > + btrfs_put_bbio(bbio); > + goto out; > + } > > /* > * Just in case we get back EOPNOTSUPP for some > reason, > @@ -1354,7 +1372,9 @@ int btrfs_discard_extent(struct btrfs_fs_info *fs_info, > u64 bytenr, > ret = 0; > } > btrfs_put_bbio(bbio); > + cur += num_bytes; > } > +out: > btrfs_bio_counter_dec(fs_info); > > if (actual_bytes) > -- > 2.23.0 > -- Filipe David Manana, “Whether you think you can, or you think you can't — you're right.”
