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

[Li Chen]

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?

Regards,
Li​