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, The xfstests pass for sub page size as well as bs = page size for POWERPC with no new regressions. 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? Also, just curious whether you came across this bug during code reading or were you actually hitting it? Regards, Ojaswin >
