From: "Darrick J. Wong" <darrick.wong@oracle.com> To: "Theodore Y. Ts'o" <tytso@mit.edu> Cc: Christoph Hellwig <hch@infradead.org>, Ext4 Developers List <linux-ext4@vger.kernel.org>, linux-f2fs-devel@lists.sourceforge.net, linux-xfs@vger.kernel.org, Eric Biggers <ebiggers@kernel.org>, Richard Weinberger <richard@nod.at> Subject: Re: [PATCH 1/2] writeback: avoid double-writing the inode on a lazytime expiration Date: Wed, 25 Mar 2020 08:47:59 -0700 [thread overview] Message-ID: <20200325154759.GY29339@magnolia> (raw) In-Reply-To: <20200325152113.GK53396@mit.edu> On Wed, Mar 25, 2020 at 11:21:13AM -0400, Theodore Y. Ts'o wrote: > On Wed, Mar 25, 2020 at 02:20:57AM -0700, Christoph Hellwig wrote: > > > spin_unlock(&inode->i_lock); > > > > > > - if (dirty & I_DIRTY_TIME) > > > - mark_inode_dirty_sync(inode); > > > + /* This was a lazytime expiration; we need to tell the file system */ > > > + if (dirty & I_DIRTY_TIME_EXPIRED && inode->i_sb->s_op->dirty_inode) > > > + inode->i_sb->s_op->dirty_inode(inode, I_DIRTY_SYNC); > > > > I think this needs a very clear comment explaining why we don't go > > through __mark_inode_dirty. > > I can take the explanation which is in the git commit description and > move it into the comment. > > > But as said before I'd rather have a new lazytime_expired operation that > > makes it very clear what is happening. We currenly have 4 file systems > > (ext4, f2fs, ubifs and xfs) that support lazytime, so this won't really > > be a major churn. > > Again, I believe patch #2 does what you want; if it doesn't can you > explain why passing I_DIRTY_TIME_EXPIRED to s_op->dirty_inode() isn't > "a new lazytime expired operation that makes very clear what is > happening"? > > I separated out patch #1 and patch #2 because patch #1 preserves > current behavior, and patch #2 modifies XFS code, which I don't want > to push Linus without an XFS reviewed-by. > > N.b. None of the other file systems required a change for patch #2, > so if you want, we can have the XFS tree carry patch #2, and/or > combine that with whatever other simplifying changes that you want. > Or I can combine patch #1 and patch #2, with an XFS Reviewed-by, and > send it through the ext4 tree. > > What's your pleasure? TBH while I'm pretty sure this does actually maintain more or less the same behavior on xfs, I prefer Christoph's explicit ->lazytime_expired approach[1] over squinting at bitflag manipulations. (It also took me a while to realize that this patch duo even existed, as it was kinda buried in its parent thread...) --D [1] https://lore.kernel.org/linux-fsdevel/20200325122825.1086872-1-hch@lst.de/T/#t > > - Ted >
WARNING: multiple messages have this Message-ID (diff)
From: "Darrick J. Wong" <darrick.wong@oracle.com> To: "Theodore Y. Ts'o" <tytso@mit.edu> Cc: linux-xfs@vger.kernel.org, Eric Biggers <ebiggers@kernel.org>, Richard Weinberger <richard@nod.at>, linux-f2fs-devel@lists.sourceforge.net, Christoph Hellwig <hch@infradead.org>, Ext4 Developers List <linux-ext4@vger.kernel.org> Subject: Re: [f2fs-dev] [PATCH 1/2] writeback: avoid double-writing the inode on a lazytime expiration Date: Wed, 25 Mar 2020 08:47:59 -0700 [thread overview] Message-ID: <20200325154759.GY29339@magnolia> (raw) In-Reply-To: <20200325152113.GK53396@mit.edu> On Wed, Mar 25, 2020 at 11:21:13AM -0400, Theodore Y. Ts'o wrote: > On Wed, Mar 25, 2020 at 02:20:57AM -0700, Christoph Hellwig wrote: > > > spin_unlock(&inode->i_lock); > > > > > > - if (dirty & I_DIRTY_TIME) > > > - mark_inode_dirty_sync(inode); > > > + /* This was a lazytime expiration; we need to tell the file system */ > > > + if (dirty & I_DIRTY_TIME_EXPIRED && inode->i_sb->s_op->dirty_inode) > > > + inode->i_sb->s_op->dirty_inode(inode, I_DIRTY_SYNC); > > > > I think this needs a very clear comment explaining why we don't go > > through __mark_inode_dirty. > > I can take the explanation which is in the git commit description and > move it into the comment. > > > But as said before I'd rather have a new lazytime_expired operation that > > makes it very clear what is happening. We currenly have 4 file systems > > (ext4, f2fs, ubifs and xfs) that support lazytime, so this won't really > > be a major churn. > > Again, I believe patch #2 does what you want; if it doesn't can you > explain why passing I_DIRTY_TIME_EXPIRED to s_op->dirty_inode() isn't > "a new lazytime expired operation that makes very clear what is > happening"? > > I separated out patch #1 and patch #2 because patch #1 preserves > current behavior, and patch #2 modifies XFS code, which I don't want > to push Linus without an XFS reviewed-by. > > N.b. None of the other file systems required a change for patch #2, > so if you want, we can have the XFS tree carry patch #2, and/or > combine that with whatever other simplifying changes that you want. > Or I can combine patch #1 and patch #2, with an XFS Reviewed-by, and > send it through the ext4 tree. > > What's your pleasure? TBH while I'm pretty sure this does actually maintain more or less the same behavior on xfs, I prefer Christoph's explicit ->lazytime_expired approach[1] over squinting at bitflag manipulations. (It also took me a while to realize that this patch duo even existed, as it was kinda buried in its parent thread...) --D [1] https://lore.kernel.org/linux-fsdevel/20200325122825.1086872-1-hch@lst.de/T/#t > > - Ted > _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
next prev parent reply other threads:[~2020-03-25 15:48 UTC|newest] Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-03-06 0:45 lazytime causing inodes to remain dirty after sync? Eric Biggers 2020-03-06 0:45 ` [f2fs-dev] " Eric Biggers 2020-03-07 2:00 ` [PATCH] writeback: avoid double-writing the inode on a lazytime expiration Theodore Ts'o 2020-03-07 2:00 ` [f2fs-dev] " Theodore Ts'o 2020-03-11 3:20 ` Eric Biggers 2020-03-11 3:20 ` [f2fs-dev] " Eric Biggers 2020-03-11 12:57 ` Theodore Y. Ts'o 2020-03-11 12:57 ` [f2fs-dev] " Theodore Y. Ts'o 2020-03-12 0:07 ` Dave Chinner 2020-03-12 0:07 ` [f2fs-dev] " Dave Chinner 2020-03-12 14:34 ` Christoph Hellwig 2020-03-12 14:34 ` [f2fs-dev] " Christoph Hellwig 2020-03-12 22:39 ` Dave Chinner 2020-03-12 22:39 ` [f2fs-dev] " Dave Chinner 2020-03-20 2:46 ` Theodore Y. Ts'o 2020-03-20 2:46 ` [f2fs-dev] " Theodore Y. Ts'o 2020-03-20 2:52 ` [PATCH 1/2] " Theodore Ts'o 2020-03-20 2:52 ` [f2fs-dev] " Theodore Ts'o 2020-03-20 2:52 ` [PATCH 2/2] writeback, xfs: call dirty_inode() with I_DIRTY_TIME_EXPIRED when appropriate Theodore Ts'o 2020-03-20 2:52 ` [f2fs-dev] " Theodore Ts'o 2020-03-23 17:58 ` Theodore Y. Ts'o 2020-03-23 17:58 ` [f2fs-dev] " Theodore Y. Ts'o 2020-03-24 8:37 ` Christoph Hellwig 2020-03-24 8:37 ` [f2fs-dev] " Christoph Hellwig 2020-03-24 18:43 ` Theodore Y. Ts'o 2020-03-24 18:43 ` [f2fs-dev] " Theodore Y. Ts'o 2020-03-25 9:20 ` [PATCH 1/2] writeback: avoid double-writing the inode on a lazytime expiration Christoph Hellwig 2020-03-25 9:20 ` [f2fs-dev] " Christoph Hellwig 2020-03-25 15:21 ` Theodore Y. Ts'o 2020-03-25 15:21 ` [f2fs-dev] " Theodore Y. Ts'o 2020-03-25 15:47 ` Darrick J. Wong [this message] 2020-03-25 15:47 ` Darrick J. Wong 2020-03-11 23:54 ` [PATCH] " Dave Chinner 2020-03-11 23:54 ` [f2fs-dev] " Dave Chinner
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20200325154759.GY29339@magnolia \ --to=darrick.wong@oracle.com \ --cc=ebiggers@kernel.org \ --cc=hch@infradead.org \ --cc=linux-ext4@vger.kernel.org \ --cc=linux-f2fs-devel@lists.sourceforge.net \ --cc=linux-xfs@vger.kernel.org \ --cc=richard@nod.at \ --cc=tytso@mit.edu \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.