On Mon, Jun 08, 2020 at 12:45:03PM -0400, Brian Foster wrote:
> On Thu, Jun 04, 2020 at 05:46:01PM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@xxxxxxxxxx>
> >
> > Rather than attach inodes to the cluster buffer just when we are
> > doing IO, attach the inodes to the cluster buffer when they are
> > dirtied. The means the buffer always carries a list of dirty inodes
> > that reference it, and we can use that list to make more fundamental
> > changes to inode writeback that aren't otherwise possible.
> >
> > Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> > Reviewed-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> > ---
> > fs/xfs/libxfs/xfs_trans_inode.c | 9 ++++++---
> > fs/xfs/xfs_buf_item.c | 1 +
> > fs/xfs/xfs_icache.c | 1 +
> > fs/xfs/xfs_inode.c | 24 +++++-------------------
> > fs/xfs/xfs_inode_item.c | 16 ++++++++++++++--
> > 5 files changed, 27 insertions(+), 24 deletions(-)
> >
> ...
> > diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
> > index 64bdda72f7b27..697248b7eb2be 100644
> > --- a/fs/xfs/xfs_inode_item.c
> > +++ b/fs/xfs/xfs_inode_item.c
> > @@ -660,6 +660,10 @@ xfs_inode_item_destroy(
> > * list for other inodes that will run this function. We remove them from the
> > * buffer list so we can process all the inode IO completions in one AIL lock
> > * traversal.
> > + *
> > + * Note: Now that we attach the log item to the buffer when we first log the
> > + * inode in memory, we can have unflushed inodes on the buffer list here. These
> > + * inodes will have a zero ili_last_fields, so skip over them here.
> > */
> > void
> > xfs_iflush_done(
> > @@ -677,12 +681,15 @@ xfs_iflush_done(
> > */
> > list_for_each_entry_safe(lip, n, &bp->b_li_list, li_bio_list) {
> > iip = INODE_ITEM(lip);
> > +
> > if (xfs_iflags_test(iip->ili_inode, XFS_ISTALE)) {
> > - list_del_init(&lip->li_bio_list);
> > xfs_iflush_abort(iip->ili_inode);
> > continue;
> > }
> >
> > + if (!iip->ili_last_fields)
> > + continue;
> > +
>
> If I follow the comment above this is essentially a proxy for checking
> whether the inode is flushed. IOW, could this eventually be replaced
> with the state flag check based on the cleanup discussed in the previous
> patch, right?
Yes, likely it can.
> Otherwise LGTM:
>
> Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>
Thanks!
-Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
