Re: [PATCH 07/10] fs: add a ->sync_lazytime method
Christoph Hellwig <[email protected]>
| Newsgroups | dev.linux.lists.gfs2,org.infradead.lists.linux-mtd,org.kernel.vger.io-uring,org.kernel.vger.linux-btrfs,org.kernel.vger.linux-fsdevel,org.kernel.vger.linux-kernel,org.kernel.vger.linux-nfs,org.kernel.vger.linux-unionfs,org.kernel.vger.linux-xfs |
|---|---|
| Message-ID | <[email protected]> |
On Wed, Dec 17, 2025 at 01:30:18PM +0100, Jan Kara wrote:
> > if (flags & I_DIRTY_INODE) {
> > + bool was_dirty_time =
> > + inode_state_read_once(inode) & I_DIRTY_TIME;
> > +
> > /*
> > * Inode timestamp update will piggback on this dirtying.
> > * We tell ->dirty_inode callback that timestamps need to
> > * be updated by setting I_DIRTY_TIME in flags.
> > */
> > - if (inode_state_read_once(inode) & I_DIRTY_TIME) {
> > + if (was_dirty_time) {
> > spin_lock(&inode->i_lock);
> > if (inode_state_read(inode) & I_DIRTY_TIME) {
> > inode_state_clear(inode, I_DIRTY_TIME);
> > flags |= I_DIRTY_TIME;
> > + was_dirty_time = true;
>
> This looks bogus. was_dirty_time is already true here. What I think you
> wanted here is to set it to false if locked I_DIRTY_TIME check failed.
> Otherwise the patch looks good.
Or better set it to false at initialization time and only set it to
true here to simply things a bit. But otherwise: yes.