Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: Re: [PATCH 4/5] jbd2: Fix wrongly judgement for buffer head removing
 while doing checkpoint
To:     Zhihao Cheng <chengzhihao1@huawei.com>, Jan Kara <jack@suse.cz>
Cc:     linux-ext4@vger.kernel.org, tytso@mit.edu,
        adilger.kernel@dilger.ca, yi.zhang@huawei.com, yukuai3@huawei.com
References: <20230531115100.2779605-1-yi.zhang@huaweicloud.com>
 <20230531115100.2779605-5-yi.zhang@huaweicloud.com>
 <20230601094156.m4b7rxntmaxc5zy7@quack3>
 <d73ecd71-cb4f-921f-2284-d756c68e084c@huawei.com>
From:   Zhang Yi <yi.zhang@huaweicloud.com>
Message-ID: <ee706b3e-2a87-fa1a-570b-64870d5e49ad@huaweicloud.com>
Date:   Thu, 1 Jun 2023 22:20:38 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.12.0
MIME-Version: 1.0
In-Reply-To: <d73ecd71-cb4f-921f-2284-d756c68e084c@huawei.com>
Content-Type: text/plain; charset=UTF-8
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 2023/6/1 21:44, Zhihao Cheng wrote:
> 在 2023/6/1 17:41, Jan Kara 写道:
> 
> Hi, Jan
>> On Wed 31-05-23 19:50:59, Zhang Yi wrote:
>>> From: Zhihao Cheng <chengzhihao1@huawei.com>
>>>
>>> Following process,
>>>
>>> jbd2_journal_commit_transaction
>>> // there are several dirty buffer heads in transaction->t_checkpoint_list
>>>            P1                   wb_workfn
>>> jbd2_log_do_checkpoint
>>>   if (buffer_locked(bh)) // false
>>>                              __block_write_full_page
>>>                               trylock_buffer(bh)
>>>                               test_clear_buffer_dirty(bh)
>>>   if (!buffer_dirty(bh))
>>>    __jbd2_journal_remove_checkpoint(jh)
>>>     if (buffer_write_io_error(bh)) // false
>>>                               >> bh IO error occurs <<
>>>   jbd2_cleanup_journal_tail
>>>    __jbd2_update_log_tail
>>>     jbd2_write_superblock
>>>     // The bh won't be replayed in next mount.
>>> , which could corrupt the ext4 image, fetch a reproducer in [Link].
>>>
>>> Since writeback process clears buffer dirty after locking buffer head,
>>> we can fix it by checking buffer dirty firstly and then checking buffer
>>> locked, the buffer head can be removed if it is neither dirty nor locked.
>>>
>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217490
>>> Fixes: 470decc613ab ("[PATCH] jbd2: initial copy of files from jbd")
>>> Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
>>> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
>>
>> OK, the analysis is correct but I'm afraid the fix won't be that easy.  The
>> reordering of tests you did below doesn't really help because CPU or the
>> compiler are free to order the loads (and stores) in whatever way they
>> wish. You'd have to use memory barriers when reading and modifying bh flags
>> (although the modification side is implicitely handled by the bitlock
>> code) to make this work reliably. But that is IMHO too subtle for this
>> code.
>>
> 

I write two litmus-test files in tools/memory-model to check the memory module
of these two cases as jbd2_log_do_checkpoint() and __cp_buffer_busy() does.

**1. test-ww-rr.litmus**  //simulate our changes in jbd2_log_do_checkpoint()

  C WW-RR

  (*
   * Result: Never
   *
   * This test shows that write-write ordering
   * (in P0()) is visible to external process P2().
   *
   * bit 0: lock
   * bit 1: dirty
   *)

  {
          x=2;
  }

  P0(int *x)
  {
          int r1;

          r1 = READ_ONCE(*x);
          r1 = r1 | 1;  //lock
          WRITE_ONCE(*x, r1);

          r1 = READ_ONCE(*x);
          r1 = r1 & 253;  //&~2, clear dirty
          WRITE_ONCE(*x, r1);
  }

  P1(int *x)
  {
          int r0;
          int r1;

          r0 = READ_ONCE(*x);
          r1 = READ_ONCE(*x);
  }

  exists (1:r1=2 /\ 1:r0=1)

The test results are:

  $ herd7 -conf linux-kernel.cfg litmus-tests/test-ww-rr.litmus
  Test WW-RR Allowed
  States 6
  1:r0=1; 1:r1=1;
  1:r0=2; 1:r1=1;
  1:r0=2; 1:r1=2;
  1:r0=2; 1:r1=3;
  1:r0=3; 1:r1=1;
  1:r0=3; 1:r1=3;
  No
  Witnesses
  Positive: 0 Negative: 6
  Condition exists (1:r1=2 /\ 1:r0=1)
  Observation WW-RR Never 0 6
  Time WW-RR 0.02
  Hash=d91ecee2379f8ac878b8d06f17967874

**2. test-ww-r.litmus**  //simulate our changes in __cp_buffer_busy()

  C WW-R

  (*
   * Result: Never
   *
   * This test shows that write-write ordering
   * (in P0()) is visible to external process P2().
   *
   * bit 0: lock
   * bit 1: dirty
   *)

  {
          x=2;
  }

  P0(int *x)
  {
          int r1;

          r1 = READ_ONCE(*x);
          r1 = r1 | 1;  //lock
          WRITE_ONCE(*x, r1);

          r1 = READ_ONCE(*x);
          r1 = r1 & 253;  //&~2, clear dirty
          WRITE_ONCE(*x, r1);
  }

  P1(int *x)
  {
          int r0;

          r0 = READ_ONCE(*x);
  }

  exists (1:r0=0)

The test results are:

  $ herd7 -conf linux-kernel.cfg litmus-tests/test-ww-r.litmus
  Test WW-R Allowed
  States 3
  1:r0=1;
  1:r0=2;
  1:r0=3;
  No
  Witnesses
  Positive: 0 Negative: 3
  Condition exists (1:r0=0)
  Observation WW-R Never 0 3
  Time WW-R 0.01
  Hash=d76bb39c367dc0cbd0c363a87c6c9eb7

So it looks like the out-of-order situation cannot happen, am I miss something?

Thanks,
Yi.

> Do you mean there might be a sequence like following:
> 
> jbd2_log_do_checkpoint
>  if (buffer_dirty(bh))
>  else if (buffer_locked(bh))
>  else
>    __jbd2_journal_remove_checkpoint(jh)
> 
> CPU re-arranges the order of getting buffer state.
> reg_1 = buffer_locked(bh)  // false
>                            lock_buffer(bh)
>                            clear_buffer(bh)
> reg_2 = buffer_dirty(bh)   // false
> 
> Then, jbd2_log_do_checkpoint() could become:
> if (reg_2)
> else if (reg_1)
> else
>   __jbd2_journal_remove_checkpoint(jh)  // enter !
> 
> Am I understanding right?
> 
>> What we should be doing to avoid these races is to lock the bh. So
>> something like:
>>
>>     if (jh->b_transaction != NULL) {
>>         do stuff
>>     }
>>     if (!trylock_buffer(bh)) {
>>         buffer_locked() branch
>>     }
>>     ... Now we have the buffer locked and can safely check for dirtyness
>>
>> And we need to do a similar treatment for journal_clean_one_cp_list() and
>> journal_shrink_one_cp_list().
>>
>> BTW, I think we could merge journal_clean_one_cp_list() and
>> journal_shrink_one_cp_list() into a single common function. I think we can
>> drop the nr_to_scan argument and just always cleanup the whole checkpoint
>> list and return the number of freed buffers. That way we have one less
>> function to deal with checkpoint list cleaning.
>>
>> Thinking about it some more maybe we can have a function like:
>>
>> int jbd2_try_remove_checkpoint(struct journal_head *jh)
>> {
>>     struct buffer_head *bh = jh2bh(jh);
>>
>>     if (!trylock_buffer(bh) || buffer_dirty(bh))
>>         return -EBUSY;
>>     /*
>>      * Buffer is clean and the IO has finished (we hold the buffer lock) so
>>      * the checkpoint is done. We can safely remove the buffer from this
>>      * transaction.
>>      */
>>     unlock_buffer(bh);
>>     return __jbd2_journal_remove_checkpoint(jh);
>> }
>>
>> and that can be used with a bit of care in the checkpointing functions as
>> well as in jbd2_journal_forget(), __journal_try_to_free_buffer(),
>> journal_unmap_buffer().
>>
>>                                 Honza
>>