Re: [PATCH 06/10] btrfs: don't use btrfs_bio_wq_end_io for compressed writes

[Nikolay Borisov <nborisov@suse.com>]

On 11.05.22 г. 22:20 ч., Nikolay Borisov wrote:
> 
> 
> On 4.05.22 г. 15:25 ч., Christoph Hellwig wrote:
>> Compressed write bio completion is the only user of btrfs_bio_wq_end_io
>> for writes, and the use of btrfs_bio_wq_end_io is a little suboptimal
>> here as we only real need user context for the final completion of a
>> compressed_bio structure, and not every single bio completion.
>>
>> Add a work_struct to struct compressed_bio instead and use that to call
>> finish_compressed_bio_write.  This allows to remove all handling of
>> write bios in the btrfs_bio_wq_end_io infrastructure.
>>
>> Signed-off-by: Christoph Hellwig <hch@lst.de>

<snip>


>> @@ -771,6 +762,9 @@ blk_status_t btrfs_bio_wq_end_io(struct 
>> btrfs_fs_info *info, struct bio *bio,
>>   {
>>       struct btrfs_end_io_wq *end_io_wq;
>> +    if (WARN_ON_ONCE(btrfs_op(bio) != BTRFS_MAP_WRITE))
>> +        return BLK_STS_IOERR;
> 
> This is broken w.r.t to btrfs_submit_metadata_bio since in
> that function the code is:
> 
>   if (btrfs_op(bio) != BTRFS_MAP_WRITE) {
>                        /*
>                         * called for a read, do the setup so that 
> checksum validation
>                         * can happen in the async kernel threads
>                         */
>                      ret = btrfs_bio_wq_end_io(fs_info, bio,
>                                                  BTRFS_WQ_ENDIO_METADATA);
>                        if (!ret)
>                                ret = btrfs_map_bio(fs_info, bio, 
> mirror_num);
> 
> 
> 
> So metadata reads  will end up in function which disallows reads..
> This results in failure to mount when reading various metadata.
> 
> I guess the correct fix is to include the following hunk from patch 8 in 
> this series:
> 
> @@ -916,14 +839,7 @@ void btrfs_submit_metadata_bio(struct inode *inode, 
> struct bio *bio, int mirror_
>       bio->bi_opf |= REQ_META;
> 
>       if (btrfs_op(bio) != BTRFS_MAP_WRITE) {
> -        /*
> -         * called for a read, do the setup so that checksum validation
> -         * can happen in the async kernel threads
> -         */
> -        ret = btrfs_bio_wq_end_io(fs_info, bio,
> -                      BTRFS_WQ_ENDIO_METADATA);
> -        if (!ret)
> -            ret = btrfs_map_bio(fs_info, bio, mirror_num);
> +        ret = btrfs_map_bio(fs_info, bio, mirror_num);
> 
> 
> 
> 
> 

Even if I apply this hunk to patch 6 I get more io failures, this time 
from btrfs_submit_data_read_bio, again there is relevant hunk in patch 8:

-	ret = btrfs_bio_wq_end_io(fs_info, bio,
-			btrfs_is_free_space_inode(BTRFS_I(inode)) ?
-			BTRFS_WQ_ENDIO_FREE_SPACE : BTRFS_WQ_ENDIO_DATA);
-	if (ret)
-		goto out;
-

And then there is another due to DIO in btrfs_submit_dio_bio's call of 
btrfs_bio_wq_end_io if we want to read.


Please fix those to ensure the series is actually bisectable.


<Snip>