[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZpdR4pN8IJajB9xc@li-bb2b2a4c-3307-11b2-a85c-8fa5c3a69313.ibm.com>
Date: Wed, 17 Jul 2024 10:59:57 +0530
From: Ojaswin Mujoo <ojaswin@...ux.ibm.com>
To: Baokun Li <libaokun@...weicloud.com>
Cc: linux-ext4@...r.kernel.org, tytso@....edu, adilger.kernel@...ger.ca,
jack@...e.cz, ritesh.list@...il.com, linux-kernel@...r.kernel.org,
yi.zhang@...wei.com, yangerkun@...wei.com,
Baokun Li <libaokun1@...wei.com>,
zhanchengbin <zhanchengbin1@...wei.com>
Subject: Re: [PATCH 02/20] ext4: prevent partial update of the extents path
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
> modified during the error process need to be checked.
>
> Regards,
> Baokun
> >
> > In this case, can we have something like below:
> >
> > -----------------
> > ext4_ext_remove_space
> > err = ext4_ext_rm_idx (error, path[0].p_bh inconsistent but verified)
> > /*
> > * we release buffers of the path but path[0].p_bh is not cleaned up
> > * due to other references to it (possible?)
> > */
> >
> > ... at a later point...:
> >
> > ext4_find_extent
> > bh = read_extent_tree_block()
> > /*
> > * we get the bh that was left inconsistent previously
> > * since its verified, we dont check it again corrupting
> > * the lookup
> > */
> >
> > -----------------
> >
> > Is the above scenario possible? Or would the path[0].p_bh that was
> > corrupted previously always be reread during the subsequent
> > ext4_find_extent() lookup?
> >
> > Thanks again,
> > Ojaswin
>
Powered by blists - more mailing lists