Re: [PATCH 17/42] btrfs: only require sector size alignment for end_bio_extent_writepage()

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

On 2021/4/16 下午11:13, Josef Bacik wrote:
> On 4/15/21 1:04 AM, Qu Wenruo wrote:
>> Just like read page, for subpage support we only require sector size
>> alignment.
>>
>> So change the error message condition to only require sector alignment.
>>
>> This should not affect existing code, as for regular sectorsize ==
>> PAGE_SIZE case, we are still requiring page alignment.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   fs/btrfs/extent_io.c | 29 ++++++++++++-----------------
>>   1 file changed, 12 insertions(+), 17 deletions(-)
>>
>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
>> index 53ac22e3560f..94f8b3ffe6a7 100644
>> --- a/fs/btrfs/extent_io.c
>> +++ b/fs/btrfs/extent_io.c
>> @@ -2779,25 +2779,20 @@ static void end_bio_extent_writepage(struct 
>> bio *bio)
>>           struct page *page = bvec->bv_page;
>>           struct inode *inode = page->mapping->host;
>>           struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
>> +        const u32 sectorsize = fs_info->sectorsize;
>> -        /* We always issue full-page reads, but if some block
>> -         * in a page fails to read, blk_update_request() will
>> -         * advance bv_offset and adjust bv_len to compensate.
>> -         * Print a warning for nonzero offsets, and an error
>> -         * if they don't add up to a full page.  */
>> -        if (bvec->bv_offset || bvec->bv_len != PAGE_SIZE) {
>> -            if (bvec->bv_offset + bvec->bv_len != PAGE_SIZE)
>> -                btrfs_err(fs_info,
>> -                   "partial page write in btrfs with offset %u and 
>> length %u",
>> -                    bvec->bv_offset, bvec->bv_len);
>> -            else
>> -                btrfs_info(fs_info,
>> -                   "incomplete page write in btrfs with offset %u and 
>> length %u",
>> -                    bvec->bv_offset, bvec->bv_len);
>> -        }
>> +        /* Btrfs read write should always be sector aligned. */
>> +        if (!IS_ALIGNED(bvec->bv_offset, sectorsize))
>> +            btrfs_err(fs_info,
>> +        "partial page write in btrfs with offset %u and length %u",
>> +                  bvec->bv_offset, bvec->bv_len);
>> +        else if (!IS_ALIGNED(bvec->bv_len, sectorsize))
>> +            btrfs_info(fs_info,
>> +        "incomplete page write with offset %u and length %u",
>> +                   bvec->bv_offset, bvec->bv_len);
>> -        start = page_offset(page);
>> -        end = start + bvec->bv_offset + bvec->bv_len - 1;
>> +        start = page_offset(page) + bvec->bv_offset;
>> +        end = start + bvec->bv_len - 1;
> 
> Does this bit work out for you now?

At least generic passes here for my arm board.

>  Because before start was just the 
> page offset.  Clearly the way it was before is a bug (I think?), because 
> it gets used in btrfs_writepage_endio_finish_ordered() with the 
> start+len, so you really do want start = page_offset(page) + bv_offset.  

Not a bug before, as for sectorsize == PAGE_SIZE case, all bvec has 
bv_offset == 0 and bv_len == PAGE_SIZE.

Thanks,
Qu

> But this is a behavior change that warrants a patch of its own as it's 
> unrelated to the sectorsize change.  (Yes I realize I'm asking for more 
> patches in an already huge series, yes I'm insane.) Thanks,
> 
> Josef