Re: [PATCH 02/20] ext4: prevent partial update of the extents path

[Ojaswin Mujoo]

On Thu, Jul 25, 2024 at 01:35:10PM +0800, Baokun Li wrote:
> On 2024/7/24 14:23, Ojaswin Mujoo wrote:
> > On Wed, Jul 17, 2024 at 02:11:27PM +0800, Baokun Li wrote:
> > > On 2024/7/17 13:29, Ojaswin Mujoo wrote:
> > > > On Tue, Jul 16, 2024 at 07:54:43PM +0800, Baokun Li wrote:
> > > > > Hi Ojaswin,
> > > > > 
> > > > > On 2024/7/16 17:54, Ojaswin Mujoo wrote:
> > > > > > > > But the journal will ensure the consistency of the extents path after
> > > > > > > > this patch.
> > > > > > > > 
> > > > > > > > When ext4_ext_get_access() or ext4_ext_dirty() returns an error in
> > > > > > > > ext4_ext_rm_idx() and ext4_ext_correct_indexes(), this may cause
> > > > > > > > the extents tree to be inconsistent. But the inconsistency just
> > > > > > > > exists in memory and doesn't land on disk.
> > > > > > > > 
> > > > > > > > For ext4_ext_get_access(), the handle must have been aborted
> > > > > > > > when it returned an error, as follows:
> > > > > > > ext4_ext_get_access
> > > > > > >     ext4_journal_get_write_access
> > > > > > >      __ext4_journal_get_write_access
> > > > > > >       err = jbd2_journal_get_write_access
> > > > > > >       if (err)
> > > > > > >         ext4_journal_abort_handle
> > > > > > > > For ext4_ext_dirty(), since path->p_bh must not be null and handle
> > > > > > > > must be valid, handle is aborted anyway when an error is returned:
> > > > > > > ext4_ext_dirty
> > > > > > >     __ext4_ext_dirty
> > > > > > >      if (path->p_bh)
> > > > > > >        __ext4_handle_dirty_metadata
> > > > > > >         if (ext4_handle_valid(handle))
> > > > > > >           err = jbd2_journal_dirty_metadata
> > > > > > >            if (!is_handle_aborted(handle) && WARN_ON_ONCE(err))
> > > > > > >              ext4_journal_abort_handle
> > > > > > > > Thus the extents tree will only be inconsistent in memory, so only
> > > > > > > > the verified bit of the modified buffer needs to be cleared to avoid
> > > > > > > > these inconsistent data being used in memory.
> > > > > > > > 
> > > > > > > Regards,
> > > > > > > Baokun
> > > > > > Thanks for the explanation Baokun, so basically we only have the
> > > > > > inconsitency in the memory.
> > > > > > 
> > > > > > I do have a followup questions:
> > > > > > 
> > > > > > So in the above example, after we have the error, we'll have the buffer
> > > > > > for depth=0 marked as valid but pointing to the wrong ei_block.
> > > > > It looks wrong here. When there is an error, the ei_block of the
> > > > > unmodified buffer with depth=0 is the correct one, it is indeed
> > > > > 'valid' and it is consistent with the disk. Only buffers that were
> > > > Hey Baokun,
> > > > 
> > > > Ahh I see now, I was looking at it the wrong way. So basically since
> > > > depth 1 to 4 is inconsistent to the disk we mark then non verified so
> > > > then subsequent lookups can act accordingly.
> > > > 
> > > > Thanks for the explanation! I am in the middle of testing this patchset
> > > > with xfstests on a POWERPC system with 64k page size. I'll let you know
> > > > how that goes!
> > > > 
> > > > Regards,
> > > > Ojaswin
> > > Hi Ojaswin,
> > > 
> > > Thank you for the test and feedback!
> > > 
> > > Cheers,
> > > Baokun
> > Hey Baokun,
> 
> Hi Ojaswin,
> 
> Sorry for the slow reply, I'm currently on a business trip.
> 
> > The xfstests pass for sub page size as well as bs = page size for
> > POWERPC with no new regressions.
> Thank you very much for your test!
> > 
> > Although for this particular patch I doubt if we would be able to
> > exersice the error path using xfstests. We might need to artifically
> > inject error in ext4_ext_get_access or ext4_ext_dirty.  Do you have any
> > other way of testing this?
> The issues in this patch set can all be triggered by injecting EIO or
> ENOMEM into ext4_find_extent(). So not only did I test kvm-xftests
> several times on x86 to make sure there weren't any regressions,
> but I also tested that running kvm-xfstests while randomly injecting
> faults into ext4_find_extent() didn't crash the system.

Ahh got it, thanks. I think I understand the changes well enough now and 
it makes sense to me to mark them non verified in case of errors.
Furthermore, the tests also look fine. Feel free to add:

Reviewed-by: Ojaswin Mujoo <ojaswin@xxxxxxxxxxxxx>

> > 
> > Also, just curious whether you came across this bug during code reading
> > or were you actually hitting it?
> The initial issue was that e2fsck was always reporting some sort of
> extents tree exception after testing, so the processes in question
> were troubleshooting and hardening, i.e. the first two patches.
> The other issues were discovered during fault injection testing of
> the processes in question.
> 
> 
> Regards,
> Baokun
>