Re: [PATCH 2/9] btrfs: add compressed_bio::io_sectors to trace compressed bio more elegantly

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2021/6/11 下午12:18, Anand Jain wrote:
> On 11/6/21 9:31 am, Qu Wenruo wrote:
>> For btrfs_submit_compressed_read() and btrfs_submit_compressed_write(),
>> we have a pretty weird dance around compressed_bio::io_sectors:
> 
> I don't see io_sectors member for the struct compressed_bio in the 
> current misc-next. It is not mentioned in the cover letter, but is this 
> series a part of some other patch set yet to integrate into misc-next? 
> OR can this series be re-based on current misc-next?

My bad, the wrong variable name in the commit message.

It should be compressed_bio::pending_bios.

The io_sectors is added to address the problem.

Thanks,
Qu
> 
> Thanks, Anand
> 
> 
>>
>>    btrfs_submit_compressed_read/write()
>>    {
>>     cb = kmalloc()
>>     refcount_set(&cb->pending_bios, 0);
>>     bio = btrfs_alloc_bio();
>>
>>     /* NOTE here, we haven't yet submitted any bio */
>>     refcount_set(&cb->pending_bios, 1);
>>
>>     for (pg_index = 0; pg_index < cb->nr_pages; pg_index++) {
>>         if (submit) {
>>             /* Here we submit bio, but we always have one
>>              * extra pending_bios */
>>             refcount_inc(&cb->pending_bios);
>>             ret = btrfs_map_bio();
>>         }
>>     }
>>
>>     /* Submit the last bio */
>>     ret = btrfs_map_bio();
>>    }
>>
>> There are two reasons why we do this:
>>
>> - compressed_bio::pending_bios is a refcount
>>    Thus if it's reduced to 0, it can not be increased again.
>>
>> - To ensure the compressed_bio is not freed by some submitted bios
>>    If the submitted bio is finished before the next bio submitted,
>>    we can free the compressed_bio completely.
>>
>> But the above code is sometimes confusing, and we can do it better by
>> just introduce a new member, compressed_bio::io_sectors.
>>
>> With that member, we can easily distinguish if we're really the last
>> bio at endio time, and even allows us to remove some BUG_ON() later,
>> as we now know how many bytes are not yet submitted.
>>
>> With this new member, now compressed_bio::pending_bios really indicates
>> the pending bios, without any special handling needed.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   fs/btrfs/compression.c | 71 ++++++++++++++++++++++--------------------
>>   fs/btrfs/compression.h | 10 +++++-
>>   2 files changed, 47 insertions(+), 34 deletions(-)
>>
>> diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
>> index fc4f37adb7b7..c9dbe306f6ba 100644
>> --- a/fs/btrfs/compression.c
>> +++ b/fs/btrfs/compression.c
>> @@ -193,6 +193,34 @@ static int check_compressed_csum(struct 
>> btrfs_inode *inode, struct bio *bio,
>>       return 0;
>>   }
>> +/*
>> + * Reduce bio and io accounting for a compressed_bio with its 
>> coresponding bio.
>> + *
>> + * Return true if there is no pending bio nor io.
>> + * Return false otherwise.
>> + */
>> +static bool dec_and_test_compressed_bio(struct compressed_bio *cb,
>> +                    struct bio *bio)
>> +{
>> +    struct btrfs_fs_info *fs_info = btrfs_sb(cb->inode->i_sb);
>> +    unsigned int bi_size = bio->bi_iter.bi_size;
>> +    bool last_bio = false;
>> +    bool last_io = false;
>> +
>> +    if (bio->bi_status)
>> +        cb->errors = 1;
>> +
>> +    last_bio = atomic_dec_and_test(&cb->pending_bios);
>> +    last_io = atomic_sub_and_test(bi_size >> fs_info->sectorsize_bits,
>> +                       &cb->io_sectors);
>> +
>> +    /*
>> +     * We can only finish the compressed bio if no pending bio and 
>> all io
>> +     * submitted.
>> +     */
>> +    return last_bio && last_io;
>> +}
>> +
>>   /* when we finish reading compressed pages from the disk, we
>>    * decompress them and then run the bio end_io routines on the
>>    * decompressed pages (in the inode address space).
>> @@ -212,13 +240,7 @@ static void end_compressed_bio_read(struct bio *bio)
>>       unsigned int mirror = btrfs_io_bio(bio)->mirror_num;
>>       int ret = 0;
>> -    if (bio->bi_status)
>> -        cb->errors = 1;
>> -
>> -    /* if there are more bios still pending for this compressed
>> -     * extent, just exit
>> -     */
>> -    if (!refcount_dec_and_test(&cb->pending_bios))
>> +    if (!dec_and_test_compressed_bio(cb, bio))
>>           goto out;
>>       /*
>> @@ -336,13 +358,7 @@ static void end_compressed_bio_write(struct bio 
>> *bio)
>>       struct page *page;
>>       unsigned long index;
>> -    if (bio->bi_status)
>> -        cb->errors = 1;
>> -
>> -    /* if there are more bios still pending for this compressed
>> -     * extent, just exit
>> -     */
>> -    if (!refcount_dec_and_test(&cb->pending_bios))
>> +    if (!dec_and_test_compressed_bio(cb, bio))
>>           goto out;
>>       /* ok, we're the last bio for this extent, step one is to
>> @@ -408,7 +424,8 @@ blk_status_t btrfs_submit_compressed_write(struct 
>> btrfs_inode *inode, u64 start,
>>       cb = kmalloc(compressed_bio_size(fs_info, compressed_len), 
>> GFP_NOFS);
>>       if (!cb)
>>           return BLK_STS_RESOURCE;
>> -    refcount_set(&cb->pending_bios, 0);
>> +    atomic_set(&cb->pending_bios, 0);
>> +    atomic_set(&cb->io_sectors, compressed_len >> 
>> fs_info->sectorsize_bits);
>>       cb->errors = 0;
>>       cb->inode = &inode->vfs_inode;
>>       cb->start = start;
>> @@ -441,7 +458,6 @@ blk_status_t btrfs_submit_compressed_write(struct 
>> btrfs_inode *inode, u64 start,
>>           bio->bi_opf |= REQ_CGROUP_PUNT;
>>           kthread_associate_blkcg(blkcg_css);
>>       }
>> -    refcount_set(&cb->pending_bios, 1);
>>       /* create and submit bios for the compressed pages */
>>       bytes_left = compressed_len;
>> @@ -462,13 +478,7 @@ blk_status_t btrfs_submit_compressed_write(struct 
>> btrfs_inode *inode, u64 start,
>>           page->mapping = NULL;
>>           if (submit || len < PAGE_SIZE) {
>> -            /*
>> -             * inc the count before we submit the bio so
>> -             * we know the end IO handler won't happen before
>> -             * we inc the count.  Otherwise, the cb might get
>> -             * freed before we're done setting it up
>> -             */
>> -            refcount_inc(&cb->pending_bios);
>> +            atomic_inc(&cb->pending_bios);
>>               ret = btrfs_bio_wq_end_io(fs_info, bio,
>>                             BTRFS_WQ_ENDIO_DATA);
>>               BUG_ON(ret); /* -ENOMEM */
>> @@ -506,6 +516,7 @@ blk_status_t btrfs_submit_compressed_write(struct 
>> btrfs_inode *inode, u64 start,
>>           cond_resched();
>>       }
>> +    atomic_inc(&cb->pending_bios);
>>       ret = btrfs_bio_wq_end_io(fs_info, bio, BTRFS_WQ_ENDIO_DATA);
>>       BUG_ON(ret); /* -ENOMEM */
>> @@ -689,7 +700,8 @@ blk_status_t btrfs_submit_compressed_read(struct 
>> inode *inode, struct bio *bio,
>>       if (!cb)
>>           goto out;
>> -    refcount_set(&cb->pending_bios, 0);
>> +    atomic_set(&cb->pending_bios, 0);
>> +    atomic_set(&cb->io_sectors, compressed_len >> 
>> fs_info->sectorsize_bits);
>>       cb->errors = 0;
>>       cb->inode = inode;
>>       cb->mirror_num = mirror_num;
>> @@ -734,7 +746,6 @@ blk_status_t btrfs_submit_compressed_read(struct 
>> inode *inode, struct bio *bio,
>>       comp_bio->bi_opf = REQ_OP_READ;
>>       comp_bio->bi_private = cb;
>>       comp_bio->bi_end_io = end_compressed_bio_read;
>> -    refcount_set(&cb->pending_bios, 1);
>>       for (pg_index = 0; pg_index < nr_pages; pg_index++) {
>>           u32 pg_len = PAGE_SIZE;
>> @@ -763,18 +774,11 @@ blk_status_t btrfs_submit_compressed_read(struct 
>> inode *inode, struct bio *bio,
>>           if (submit || bio_add_page(comp_bio, page, pg_len, 0) < 
>> pg_len) {
>>               unsigned int nr_sectors;
>> +            atomic_inc(&cb->pending_bios);
>>               ret = btrfs_bio_wq_end_io(fs_info, comp_bio,
>>                             BTRFS_WQ_ENDIO_DATA);
>>               BUG_ON(ret); /* -ENOMEM */
>> -            /*
>> -             * inc the count before we submit the bio so
>> -             * we know the end IO handler won't happen before
>> -             * we inc the count.  Otherwise, the cb might get
>> -             * freed before we're done setting it up
>> -             */
>> -            refcount_inc(&cb->pending_bios);
>> -
>>               ret = btrfs_lookup_bio_sums(inode, comp_bio, sums);
>>               BUG_ON(ret); /* -ENOMEM */
>> @@ -798,6 +802,7 @@ blk_status_t btrfs_submit_compressed_read(struct 
>> inode *inode, struct bio *bio,
>>           cur_disk_byte += pg_len;
>>       }
>> +    atomic_inc(&cb->pending_bios);
>>       ret = btrfs_bio_wq_end_io(fs_info, comp_bio, BTRFS_WQ_ENDIO_DATA);
>>       BUG_ON(ret); /* -ENOMEM */
>> diff --git a/fs/btrfs/compression.h b/fs/btrfs/compression.h
>> index 8001b700ea3a..3df3262fedcd 100644
>> --- a/fs/btrfs/compression.h
>> +++ b/fs/btrfs/compression.h
>> @@ -29,7 +29,15 @@ struct btrfs_inode;
>>   struct compressed_bio {
>>       /* number of bios pending for this compressed extent */
>> -    refcount_t pending_bios;
>> +    atomic_t pending_bios;
>> +
>> +    /*
>> +     * Number of sectors which hasn't finished.
>> +     *
>> +     * Combined with pending_bios, we can manually finish the 
>> compressed_bio
>> +     * if we hit some error while there is still some pages not added.
>> +     */
>> +    atomic_t io_sectors;
>>       /* the pages with the compressed data on them */
>>       struct page **compressed_pages;
>>
>