Re: [PATCH v2 1/3] jbd2: store jinode dirty range in PAGE_SIZE units

[Andreas Dilger]

On Feb 19, 2026, at 23:43, Li Chen <me@linux.beauty> wrote:
> 
> Hi Andreas,
> 
> Thanks a lot for your review!
> 
> ---- On Fri, 20 Feb 2026 05:00:13 +0800  Andreas Dilger <adilger@xxxxxxxxx> wrote ---
>> On Feb 19, 2026, at 04:46, Li Chen <me@linux.beauty> wrote:
>>> 
>>> jbd2_inode fields are updated under journal->j_list_lock, but some paths
>>> read them without holding the lock (e.g. fast commit helpers and ordered
>>> truncate helpers).
>>> 
>>> READ_ONCE() alone is not sufficient for i_dirty_start/end as they are
>>> loff_t and 32-bit platforms can observe torn loads. Store the dirty range
>>> in PAGE_SIZE units as pgoff_t so lockless readers can take non-torn
>>> snapshots.
>> 
>> When making semantic changes like this, it is best to change the variable
>> names as well, so that breaks compilation if bisection happens to land
>> between these patches.  Otherwise, that could cause some random behavior
>> if jbd2 is treating these as pages, but ext4/ocfs2 are treating them as
>> bytes or vice versa.
>> 
>> Something like i_dirty_{start,end} -> i_dirty_{start,end}_page would make
>> this very clear what the units are.
> 
> Agreed. I’ll make the units explicit in the field names (e.g. *_page).
> 
>> To avoid breakage between the patches (which is desirable to avoid problems
>> with automated bisection) you should make an initial patch with wrappers to
>> access these values and convert ext4/ocfs2 to use them:
>> 
>> static inline loff_t jbd2_jinode_dirty_start(struct jbd2_inode *jinode)
>> {
>>    return jinode->i_dirty_start;
>> }
>> 
>> static inline loff_t jbd2_jinode_dirty_end(struct jbd2_inode *jinode)
>> {
>>    return jinode->i_dirty_end;
>> }
>> 
>> then change this in the jbd2 patch at the end, which would then be self-contained:
>> 
>> static inline loff_t jbd2_jinode_dirty_start(struct jbd2_inode *jinode)
>> {
>>    return (loff_t)jinode->i_dirty_start_page << PAGE_SHIFT;
>> }
>> 
>> static inline loff_t jbd2_jinode_dirty_end(struct jbd2_inode *jinode)
>> {
>>    return ((loff_t)jinode->i_dirty_end_page << PAGE_SHIFT) + ~PAGE_MASK;
>> }
> 
> 
> Agreed as well. I’ll add an accessor and switch ext4/ocfs2 over to it first,
> Then do the internal representation change later.
> 
> I plan to use a single helper that returns the (start,end) pair in
> bytes:
> 
> static inline bool jbd2_jinode_get_dirty_range(const struct jbd2_inode *jinode,
> loff_t *start, loff_t *end)
> {
>    pgoff_t start_page = READ_ONCE(jinode->i_dirty_start_page);
>    pgoff_t end_page = READ_ONCE(jinode->i_dirty_end_page);
> 
>    if (end_page == JBD2_INODE_DIRTY_RANGE_NONE)
>      return false;
> 
>    *start = (loff_t)start_page << PAGE_SHIFT;
>    *end = ((loff_t)end_page << PAGE_SHIFT) + PAGE_SIZE - 1;
>    return true;
> 
> }
> 
> I think this is a bit easier to use correctly than separate start/end helpers
> (keeps start/end together, and the end-of-page conversion lives in one place).
> 
> Does that sound OK, or would you rather see separate
> jbd2_jinode_dirty_start()/jbd2_jinode_dirty_end() helpers?

This is fine with me.  I had only proposed the other option as a "typical"
interface for such fields.  If start/end are always used together then it
is fine that there is only one function to get these fields, and one to
set them.

Cheers, Andreas