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;
DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws; 
  s=zapps768; d=zoho.com; 
  h=message-id:date:mime-version:user-agent:subject:to:cc:references:from:in-reply-to:content-type; 
  b=dsKVwXCus/vx4HZb8Ll7diczxweR21MnfPqtc2FJ3mUBRUyKECuXpvo5wIah+neHVNdgylrBv/Xn
    QTSEnlB8APM+Vbbrwi0JS7KyXCkK8AV5Ao7k9XGd9DgIf7bdkL45  
Message-ID: <b1687080-a702-e1f7-0fac-e837b0317aa4@zoho.com>
Date:   Thu, 29 Sep 2022 02:41:28 -0400
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH] btrfs:remove redundant index_rbio_pages in
 raid56_rmw_stripe
Content-Language: en-US
To:     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>
From:   hmsjwzb <hmsjwzb@zoho.com>
In-Reply-To: <18acd4e0-9d3d-77a7-a1de-b805bc259bcf@suse.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi Qu,

	Thanks for your reply.
	Here is my plan about destructive RMW.
	
	1. Read all the stripes in through raid56_rmw_stripe.
	2. Do xor operation in finish_rmw.
	3. If the xor result matches, nothing happened.
	4. If the xor result mismatches, we can recover the data or trigger some user space progress to fix the data corruption.

	But here are some problems.
	1. If the stripe is new allocated, the check will fail.
	2. Is it convient for kernel get checksum in finish_rmw and recover data?

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:
>>>    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 check 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_pages() 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
>>
>>>    It is invoked in finish_rmw anyway.
>>>
>>> Signed-off-by: Flint.Wang <hmsjwzb@zoho.com>
>>> ---
>>>   fs/btrfs/raid56.c | 2 --
>>>   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)
>>>       if (ret)
>>>           goto cleanup;
>>> -    index_rbio_pages(rbio);
>>> -
>>>       atomic_set(&rbio->error, 0);
>>>       /* Build a list of bios to read all the missing data sectors. */
>>>       for (total_sector_nr = 0; total_sector_nr < nr_data_sectors;