From: "Darrick J. Wong" <darrick.wong@oracle.com>
To: Dave Chinner <david@fromorbit.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 24/30] xfs: rework stale inodes in xfs_ifree_cluster
Date: Tue, 2 Jun 2020 16:01:12 -0700 [thread overview]
Message-ID: <20200602230112.GQ8230@magnolia> (raw)
In-Reply-To: <20200601214251.4167140-25-david@fromorbit.com>
On Tue, Jun 02, 2020 at 07:42:45AM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> Once we have inodes pinning the cluster buffer and attached whenever
> they are dirty, we no longer have a guarantee that the items are
> flush locked when we lock the cluster buffer. Hence we cannot just
> walk the buffer log item list and modify the attached inodes.
>
> If the inode is not flush locked, we have to ILOCK it first and
> the flush lock it and do all the prerequisite checks needed to avoid
"...and then flush lock it..."
> races with other code. This is already handled by
> xfs_ifree_get_one_inode(), so rework the inode iteration loop and
> function to update all inodes in cache whether they are attached to
> the buffer or not.
>
> Note: we also remove the copying of the log item lsn to the
> ili_flush_lsn as xfs_iflush_done() now uses the XFS_ISTALE flag to
> trigger aborts and so flush lsn matching is not needed in IO
> completion for processing freed inodes.
Ok. Thanks for breaking this up a bit since the previous patch. That
makes it easier to figure out what's going on.
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
With that fixed,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
--D
> ---
> fs/xfs/xfs_inode.c | 158 ++++++++++++++++++---------------------------
> 1 file changed, 62 insertions(+), 96 deletions(-)
>
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 272b54cf97000..fb4c614c64fda 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -2517,17 +2517,19 @@ xfs_iunlink_remove(
> }
>
> /*
> - * Look up the inode number specified and mark it stale if it is found. If it is
> - * dirty, return the inode so it can be attached to the cluster buffer so it can
> - * be processed appropriately when the cluster free transaction completes.
> + * Look up the inode number specified and if it is not already marked XFS_ISTALE
> + * mark it stale. We should only find clean inodes in this lookup that aren't
> + * already stale.
> */
> -static struct xfs_inode *
> -xfs_ifree_get_one_inode(
> - struct xfs_perag *pag,
> +static void
> +xfs_ifree_mark_inode_stale(
> + struct xfs_buf *bp,
> struct xfs_inode *free_ip,
> xfs_ino_t inum)
> {
> - struct xfs_mount *mp = pag->pag_mount;
> + struct xfs_mount *mp = bp->b_mount;
> + struct xfs_perag *pag = bp->b_pag;
> + struct xfs_inode_log_item *iip;
> struct xfs_inode *ip;
>
> retry:
> @@ -2535,8 +2537,10 @@ xfs_ifree_get_one_inode(
> ip = radix_tree_lookup(&pag->pag_ici_root, XFS_INO_TO_AGINO(mp, inum));
>
> /* Inode not in memory, nothing to do */
> - if (!ip)
> - goto out_rcu_unlock;
> + if (!ip) {
> + rcu_read_unlock();
> + return;
> + }
>
> /*
> * because this is an RCU protected lookup, we could find a recently
> @@ -2547,9 +2551,9 @@ xfs_ifree_get_one_inode(
> spin_lock(&ip->i_flags_lock);
> if (ip->i_ino != inum || __xfs_iflags_test(ip, XFS_ISTALE)) {
> spin_unlock(&ip->i_flags_lock);
> - goto out_rcu_unlock;
> + rcu_read_unlock();
> + return;
> }
> - spin_unlock(&ip->i_flags_lock);
>
> /*
> * Don't try to lock/unlock the current inode, but we _cannot_ skip the
> @@ -2559,43 +2563,53 @@ xfs_ifree_get_one_inode(
> */
> if (ip != free_ip) {
> if (!xfs_ilock_nowait(ip, XFS_ILOCK_EXCL)) {
> + spin_unlock(&ip->i_flags_lock);
> rcu_read_unlock();
> delay(1);
> goto retry;
> }
> -
> - /*
> - * Check the inode number again in case we're racing with
> - * freeing in xfs_reclaim_inode(). See the comments in that
> - * function for more information as to why the initial check is
> - * not sufficient.
> - */
> - if (ip->i_ino != inum) {
> - xfs_iunlock(ip, XFS_ILOCK_EXCL);
> - goto out_rcu_unlock;
> - }
> }
> + ip->i_flags |= XFS_ISTALE;
> + spin_unlock(&ip->i_flags_lock);
> rcu_read_unlock();
>
> - xfs_iflock(ip);
> - xfs_iflags_set(ip, XFS_ISTALE);
> + /*
> + * If we can't get the flush lock, the inode is already attached. All
> + * we needed to do here is mark the inode stale so buffer IO completion
> + * will remove it from the AIL.
> + */
> + iip = ip->i_itemp;
> + if (!xfs_iflock_nowait(ip)) {
> + ASSERT(!list_empty(&iip->ili_item.li_bio_list));
> + ASSERT(iip->ili_last_fields);
> + goto out_iunlock;
> + }
> + ASSERT(!iip || list_empty(&iip->ili_item.li_bio_list));
>
> /*
> - * We don't need to attach clean inodes or those only with unlogged
> - * changes (which we throw away, anyway).
> + * Clean inodes can be released immediately. Everything else has to go
> + * through xfs_iflush_abort() on journal commit as the flock
> + * synchronises removal of the inode from the cluster buffer against
> + * inode reclaim.
> */
> - if (!ip->i_itemp || xfs_inode_clean(ip)) {
> - ASSERT(ip != free_ip);
> + if (xfs_inode_clean(ip)) {
> xfs_ifunlock(ip);
> - xfs_iunlock(ip, XFS_ILOCK_EXCL);
> - goto out_no_inode;
> + goto out_iunlock;
> }
> - return ip;
>
> -out_rcu_unlock:
> - rcu_read_unlock();
> -out_no_inode:
> - return NULL;
> + /* we have a dirty inode in memory that has not yet been flushed. */
> + ASSERT(iip->ili_fields);
> + spin_lock(&iip->ili_lock);
> + iip->ili_last_fields = iip->ili_fields;
> + iip->ili_fields = 0;
> + iip->ili_fsync_fields = 0;
> + spin_unlock(&iip->ili_lock);
> + list_add_tail(&iip->ili_item.li_bio_list, &bp->b_li_list);
> + ASSERT(iip->ili_last_fields);
> +
> +out_iunlock:
> + if (ip != free_ip)
> + xfs_iunlock(ip, XFS_ILOCK_EXCL);
> }
>
> /*
> @@ -2605,26 +2619,20 @@ xfs_ifree_get_one_inode(
> */
> STATIC int
> xfs_ifree_cluster(
> - xfs_inode_t *free_ip,
> - xfs_trans_t *tp,
> + struct xfs_inode *free_ip,
> + struct xfs_trans *tp,
> struct xfs_icluster *xic)
> {
> - xfs_mount_t *mp = free_ip->i_mount;
> + struct xfs_mount *mp = free_ip->i_mount;
> + struct xfs_ino_geometry *igeo = M_IGEO(mp);
> + struct xfs_buf *bp;
> + xfs_daddr_t blkno;
> + xfs_ino_t inum = xic->first_ino;
> int nbufs;
> int i, j;
> int ioffset;
> - xfs_daddr_t blkno;
> - xfs_buf_t *bp;
> - xfs_inode_t *ip;
> - struct xfs_inode_log_item *iip;
> - struct xfs_log_item *lip;
> - struct xfs_perag *pag;
> - struct xfs_ino_geometry *igeo = M_IGEO(mp);
> - xfs_ino_t inum;
> int error;
>
> - inum = xic->first_ino;
> - pag = xfs_perag_get(mp, XFS_INO_TO_AGNO(mp, inum));
> nbufs = igeo->ialloc_blks / igeo->blocks_per_cluster;
>
> for (j = 0; j < nbufs; j++, inum += igeo->inodes_per_cluster) {
> @@ -2668,59 +2676,16 @@ xfs_ifree_cluster(
> bp->b_ops = &xfs_inode_buf_ops;
>
> /*
> - * Walk the inodes already attached to the buffer and mark them
> - * stale. These will all have the flush locks held, so an
> - * in-memory inode walk can't lock them. By marking them all
> - * stale first, we will not attempt to lock them in the loop
> - * below as the XFS_ISTALE flag will be set.
> - */
> - list_for_each_entry(lip, &bp->b_li_list, li_bio_list) {
> - if (lip->li_type == XFS_LI_INODE) {
> - iip = (struct xfs_inode_log_item *)lip;
> - xfs_trans_ail_copy_lsn(mp->m_ail,
> - &iip->ili_flush_lsn,
> - &iip->ili_item.li_lsn);
> - xfs_iflags_set(iip->ili_inode, XFS_ISTALE);
> - }
> - }
> -
> -
> - /*
> - * For each inode in memory attempt to add it to the inode
> - * buffer and set it up for being staled on buffer IO
> - * completion. This is safe as we've locked out tail pushing
> - * and flushing by locking the buffer.
> - *
> - * We have already marked every inode that was part of a
> - * transaction stale above, which means there is no point in
> - * even trying to lock them.
> + * Now we need to set all the cached clean inodes as XFS_ISTALE,
> + * too. This requires lookups, and will skip inodes that we've
> + * already marked XFS_ISTALE.
> */
> - for (i = 0; i < igeo->inodes_per_cluster; i++) {
> - ip = xfs_ifree_get_one_inode(pag, free_ip, inum + i);
> - if (!ip)
> - continue;
> -
> - iip = ip->i_itemp;
> - spin_lock(&iip->ili_lock);
> - iip->ili_last_fields = iip->ili_fields;
> - iip->ili_fields = 0;
> - iip->ili_fsync_fields = 0;
> - spin_unlock(&iip->ili_lock);
> - xfs_trans_ail_copy_lsn(mp->m_ail, &iip->ili_flush_lsn,
> - &iip->ili_item.li_lsn);
> -
> - list_add_tail(&iip->ili_item.li_bio_list,
> - &bp->b_li_list);
> -
> - if (ip != free_ip)
> - xfs_iunlock(ip, XFS_ILOCK_EXCL);
> - }
> + for (i = 0; i < igeo->inodes_per_cluster; i++)
> + xfs_ifree_mark_inode_stale(bp, free_ip, inum + i);
>
> xfs_trans_stale_inode_buf(tp, bp);
> xfs_trans_binval(tp, bp);
> }
> -
> - xfs_perag_put(pag);
> return 0;
> }
>
> @@ -3845,6 +3810,7 @@ xfs_iflush_int(
> iip->ili_fields = 0;
> iip->ili_fsync_fields = 0;
> spin_unlock(&iip->ili_lock);
> + ASSERT(iip->ili_last_fields);
>
> /*
> * Store the current LSN of the inode so that we can tell whether the
> --
> 2.26.2.761.g0e0b3e54be
>
next prev parent reply other threads:[~2020-06-02 23:03 UTC|newest]
Thread overview: 80+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-01 21:42 [PATCH 00/30] xfs: rework inode flushing to make inode reclaim fully asynchronous Dave Chinner
2020-06-01 21:42 ` [PATCH 01/30] xfs: Don't allow logging of XFS_ISTALE inodes Dave Chinner
2020-06-02 4:30 ` Darrick J. Wong
2020-06-02 7:06 ` Dave Chinner
2020-06-02 16:32 ` Brian Foster
2020-06-01 21:42 ` [PATCH 02/30] xfs: remove logged flag from inode log item Dave Chinner
2020-06-02 16:32 ` Brian Foster
2020-06-01 21:42 ` [PATCH 03/30] xfs: add an inode item lock Dave Chinner
2020-06-02 16:34 ` Brian Foster
2020-06-04 1:54 ` Dave Chinner
2020-06-04 14:03 ` Brian Foster
2020-06-01 21:42 ` [PATCH 04/30] xfs: mark inode buffers in cache Dave Chinner
2020-06-02 16:45 ` Brian Foster
2020-06-02 19:22 ` Darrick J. Wong
2020-06-02 21:29 ` Dave Chinner
2020-06-03 14:57 ` Brian Foster
2020-06-03 21:21 ` Dave Chinner
2020-06-01 21:42 ` [PATCH 05/30] xfs: mark dquot " Dave Chinner
2020-06-02 16:45 ` Brian Foster
2020-06-02 19:00 ` Darrick J. Wong
2020-06-01 21:42 ` [PATCH 06/30] xfs: mark log recovery buffers for completion Dave Chinner
2020-06-02 16:45 ` Brian Foster
2020-06-02 19:24 ` Darrick J. Wong
2020-06-01 21:42 ` [PATCH 07/30] xfs: call xfs_buf_iodone directly Dave Chinner
2020-06-02 16:47 ` Brian Foster
2020-06-02 21:38 ` Dave Chinner
2020-06-03 14:58 ` Brian Foster
2020-06-01 21:42 ` [PATCH 08/30] xfs: clean up whacky buffer log item list reinit Dave Chinner
2020-06-02 16:47 ` Brian Foster
2020-06-01 21:42 ` [PATCH 09/30] xfs: make inode IO completion buffer centric Dave Chinner
2020-06-03 14:58 ` Brian Foster
2020-06-01 21:42 ` [PATCH 10/30] xfs: use direct calls for dquot IO completion Dave Chinner
2020-06-02 19:25 ` Darrick J. Wong
2020-06-03 14:58 ` Brian Foster
2020-06-01 21:42 ` [PATCH 11/30] xfs: clean up the buffer iodone callback functions Dave Chinner
2020-06-03 14:58 ` Brian Foster
2020-06-01 21:42 ` [PATCH 12/30] xfs: get rid of log item callbacks Dave Chinner
2020-06-03 14:58 ` Brian Foster
2020-06-01 21:42 ` [PATCH 13/30] xfs: handle buffer log item IO errors directly Dave Chinner
2020-06-02 20:39 ` Darrick J. Wong
2020-06-02 22:17 ` Dave Chinner
2020-06-03 15:02 ` Brian Foster
2020-06-03 21:34 ` Dave Chinner
2020-06-01 21:42 ` [PATCH 14/30] xfs: unwind log item error flagging Dave Chinner
2020-06-02 20:45 ` Darrick J. Wong
2020-06-03 15:02 ` Brian Foster
2020-06-01 21:42 ` [PATCH 15/30] xfs: move xfs_clear_li_failed out of xfs_ail_delete_one() Dave Chinner
2020-06-02 20:47 ` Darrick J. Wong
2020-06-03 15:02 ` Brian Foster
2020-06-01 21:42 ` [PATCH 16/30] xfs: pin inode backing buffer to the inode log item Dave Chinner
2020-06-02 22:30 ` Darrick J. Wong
2020-06-02 22:53 ` Dave Chinner
2020-06-03 18:58 ` Brian Foster
2020-06-03 22:15 ` Dave Chinner
2020-06-04 14:03 ` Brian Foster
2020-06-01 21:42 ` [PATCH 17/30] xfs: make inode reclaim almost non-blocking Dave Chinner
2020-06-01 21:42 ` [PATCH 18/30] xfs: remove IO submission from xfs_reclaim_inode() Dave Chinner
2020-06-02 22:36 ` Darrick J. Wong
2020-06-01 21:42 ` [PATCH 19/30] xfs: allow multiple reclaimers per AG Dave Chinner
2020-06-01 21:42 ` [PATCH 20/30] xfs: don't block inode reclaim on the ILOCK Dave Chinner
2020-06-01 21:42 ` [PATCH 21/30] xfs: remove SYNC_TRYLOCK from inode reclaim Dave Chinner
2020-06-01 21:42 ` [PATCH 22/30] xfs: remove SYNC_WAIT from xfs_reclaim_inodes() Dave Chinner
2020-06-02 22:43 ` Darrick J. Wong
2020-06-01 21:42 ` [PATCH 23/30] xfs: clean up inode reclaim comments Dave Chinner
2020-06-02 22:45 ` Darrick J. Wong
2020-06-01 21:42 ` [PATCH 24/30] xfs: rework stale inodes in xfs_ifree_cluster Dave Chinner
2020-06-02 23:01 ` Darrick J. Wong [this message]
2020-06-01 21:42 ` [PATCH 25/30] xfs: attach inodes to the cluster buffer when dirtied Dave Chinner
2020-06-02 23:03 ` Darrick J. Wong
2020-06-01 21:42 ` [PATCH 26/30] xfs: xfs_iflush() is no longer necessary Dave Chinner
2020-06-01 21:42 ` [PATCH 27/30] xfs: rename xfs_iflush_int() Dave Chinner
2020-06-01 21:42 ` [PATCH 28/30] xfs: rework xfs_iflush_cluster() dirty inode iteration Dave Chinner
2020-06-02 23:23 ` Darrick J. Wong
2020-06-01 21:42 ` [PATCH 29/30] xfs: factor xfs_iflush_done Dave Chinner
2020-06-01 21:42 ` [PATCH 30/30] xfs: remove xfs_inobp_check() Dave Chinner
-- strict thread matches above, loose matches on Subject: below --
2020-06-04 7:45 [PATCH 00/30] xfs: rework inode flushing to make inode reclaim fully asynchronous Dave Chinner
2020-06-04 7:46 ` [PATCH 24/30] xfs: rework stale inodes in xfs_ifree_cluster Dave Chinner
2020-06-05 18:27 ` Brian Foster
2020-06-05 21:32 ` Dave Chinner
2020-06-08 16:44 ` Brian Foster
2020-06-22 8:15 [PATCH 00/30] xfs: rework inode flushing to make inode reclaim fully asynchronous Dave Chinner
2020-06-22 8:15 ` [PATCH 24/30] xfs: rework stale inodes in xfs_ifree_cluster 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=20200602230112.GQ8230@magnolia \
--to=darrick.wong@oracle.com \
--cc=david@fromorbit.com \
--cc=linux-xfs@vger.kernel.org \
/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: link
Be 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.