Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <8f1adbef-7146-c8f9-92a3-c18e10e9a66b@gmx.com>
Date:   Thu, 29 Sep 2022 15:00:03 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.2.2
Content-Language: en-US
To:     hmsjwzb <hmsjwzb@zoho.com>, Qu Wenruo <wqu@suse.com>
Cc:     Chris Mason <clm@fb.com>, Josef Bacik <josef@toxicpanda.com>,
        David Sterba <dsterba@suse.com>, linux-btrfs@vger.kernel.org,
        linux-kernel@vger.kernel.org
References: <20220929014402.2450-1-hmsjwzb@zoho.com>
 <c4293742-06ba-8720-e2eb-d4d3bc4da044@suse.com>
 <18acd4e0-9d3d-77a7-a1de-b805bc259bcf@suse.com>
 <b1687080-a702-e1f7-0fac-e837b0317aa4@zoho.com>
From:   Qu Wenruo <quwenruo.btrfs@gmx.com>
Subject: Re: [PATCH] btrfs:remove redundant index_rbio_pages in
 raid56_rmw_stripe
In-Reply-To: <b1687080-a702-e1f7-0fac-e837b0317aa4@zoho.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: quoted-printable
Precedence: bulk


On 2022/9/29 14:41, hmsjwzb wrote:
> Hi Qu,
>
> 	Thanks for your reply.
> 	Here is my plan about destructive RMW.

The overall idea is fine, but some details need to be addressed.

>
> 	1. Read all the stripes in through raid56_rmw_stripe.

Note that, even we reached raid56_rmw_stripe(), we may still be a full
stripe write (due to substripe writes merged into one full stripe).

In that case, we should not waste IO to read any stripe.

That's what your patch lacks.
(Also all the comments should be updated since you changed the behavior)

> 	2. Do xor operation in finish_rmw.

You may want to do the verification/recovery all before finish_rmw().

The function finish_rmw() is currently the last step of RMW.

> 	3. If the xor result matches, nothing happened.
> 	4. If the xor result mismatches, we can recover the data or trigger som=
e user space progress to fix the data corruption.

For now just focus on the recover part, no need to bother user space.

>
> 	But here are some problems.
> 	1. If the stripe is new allocated, the check will fail.

For now we should focus on data recovery only (metadata will be much
more complex), and for data we will need to grab checksum for the full
stripe anyway.

So in newly allocated case, there will be no checksum, thus no need to
really do any recovery, we just trust all data which doesn't has no
checksum.

> 	2. Is it convient for kernel get checksum in finish_rmw and recover dat=
a?

If not convenient, you'll need to be creative to grab the checksum.

But from what I see, if you pre-fetch the checksum for the full stripe
at raid56_parity_write() timing, it should be fine.

Thanks,
Qu

>
> Thanks,
> Flint
>
> On 9/28/22 23:13, Qu Wenruo wrote:
>>
>>
>> On 2022/9/29 11:08, Qu Wenruo wrote:
>>>
>>>
>>> On 2022/9/29 09:44, Flint.Wang wrote:
>>>>  =C2=A0=C2=A0 The index_rbio_pages in raid56_rmw_stripe is redundant.
>>>
>>> index_rbio_pages() is to populate the rbio->bio_sectors array.
>>>
>>> In raid56_rmw_stripe() we later calls sector_in_rbio(), which will che=
ck if a sector is belonging to bio_lists.
>>>
>>> If not called, all sector will be returned using the sectors in rbio->=
bio_sectors, not using the sectors in bio lists.
>>>
>>> Have you tried your patch with fstests runs?
>>
>> Well, for raid56_rmw_stripe() it's fine, as without the index_rbio_page=
s() call, we just read all the sectors from the disk.
>>
>> This would include the new pages from bio lists.
>>
>> It would only cause extra IO, but since they can all be merged into one=
 64K stripe, it should not cause performance problem.
>>
>> Furthermore it would read all old sectors from disk, allowing us later =
to do the verification before doing the writes.
>>
>> But it should really contain a more detailed explanation.
>>
>> Thanks,
>> Qu
>>>
>>> IMHO it should fail a lot of very basic writes in RAID56.
>>>
>>> Thanks,
>>> Qu
>>>
>>>>  =C2=A0=C2=A0 It is invoked in finish_rmw anyway.
>>>>
>>>> Signed-off-by: Flint.Wang <hmsjwzb@zoho.com>
>>>> ---
>>>>  =C2=A0 fs/btrfs/raid56.c | 2 --
>>>>  =C2=A0 1 file changed, 2 deletions(-)
>>>>
>>>> diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
>>>> index f6395e8288d69..44266b2c5b86e 100644
>>>> --- a/fs/btrfs/raid56.c
>>>> +++ b/fs/btrfs/raid56.c
>>>> @@ -1546,8 +1546,6 @@ static int raid56_rmw_stripe(struct btrfs_raid_=
bio *rbio)
>>>>  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (ret)
>>>>  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 goto cleanup;
>>>> -=C2=A0=C2=A0=C2=A0 index_rbio_pages(rbio);
>>>> -
>>>>  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 atomic_set(&rbio->error, 0);
>>>>  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 /* Build a list of bios to read all t=
he missing data sectors. */
>>>>  =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 for (total_sector_nr =3D 0; total_sec=
tor_nr < nr_data_sectors;