From: "Darrick J. Wong" <darrick.wong@oracle.com>
To: Dave Chinner <david@fromorbit.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 10/13] xfs: re-order AGI updates in unlink list updates
Date: Tue, 18 Aug 2020 17:29:48 -0700 [thread overview]
Message-ID: <20200819002948.GX6096@magnolia> (raw)
In-Reply-To: <20200812092556.2567285-11-david@fromorbit.com>
On Wed, Aug 12, 2020 at 07:25:53PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> We always access and check the AGI bucket entry for the unlinked
> list even if we are not going to need it either for lookup or remove
> purposes. Move the code that accesses the AGI to the code that
> modifes the AGI, hence keeping the AGI accesses local to the code
> that needs to modify it.
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
> fs/xfs/xfs_inode.c | 84 ++++++++++++++++------------------------------
> 1 file changed, 28 insertions(+), 56 deletions(-)
>
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index b098e5df07e7..4f616e1b64dc 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -1918,44 +1918,53 @@ xfs_inactive(
> */
>
> /*
> - * Point the AGI unlinked bucket at an inode and log the results. The caller
> - * is responsible for validating the old value.
> + * Point the AGI unlinked bucket at an inode and log the results. The caller
> + * passes in the expected current agino the bucket points at via @cur_agino so
> + * we can validate that we are about to remove the inode we expect to be
> + * removing from the AGI bucket.
> */
> -STATIC int
> +static int
> xfs_iunlink_update_bucket(
> struct xfs_trans *tp,
> xfs_agnumber_t agno,
> struct xfs_buf *agibp,
> - xfs_agino_t old_agino,
> + xfs_agino_t cur_agino,
Hm. So I think I understand the new role of this function better now
that this patch moves into this function the checking of the bucket
pointer and whatnot. Would it be difficult to merge this patch with
patch 4?
--D
> xfs_agino_t new_agino)
> {
> - struct xlog *log = tp->t_mountp->m_log;
> + struct xfs_mount *mp = tp->t_mountp;
> + struct xlog *log = mp->m_log;
> struct xfs_agi *agi = agibp->b_addr;
> - xfs_agino_t old_value;
> + xfs_agino_t old_agino;
> unsigned int bucket_index;
> int offset;
>
> - ASSERT(xfs_verify_agino_or_null(tp->t_mountp, agno, new_agino));
> + ASSERT(xfs_verify_agino_or_null(mp, agno, new_agino));
>
> + /*
> + * We don't need to traverse the on disk unlinked list to find the
> + * previous inode in the list when removing inodes anymore, so we don't
> + * use multiple on-disk lists anymore. Hence we always use bucket 0
> + * unless we are in log recovery in which case we might be recovering an
> + * old filesystem that has multiple buckets.
> + */
> bucket_index = 0;
> - /* During recovery, the old multiple bucket index can be applied */
> if (!log || log->l_flags & XLOG_RECOVERY_NEEDED) {
> - ASSERT(old_agino != NULLAGINO);
> + ASSERT(cur_agino != NULLAGINO);
>
> - if (be32_to_cpu(agi->agi_unlinked[0]) != old_agino)
> - bucket_index = old_agino % XFS_AGI_UNLINKED_BUCKETS;
> + if (be32_to_cpu(agi->agi_unlinked[0]) != cur_agino)
> + bucket_index = cur_agino % XFS_AGI_UNLINKED_BUCKETS;
> }
>
> - old_value = be32_to_cpu(agi->agi_unlinked[bucket_index]);
> - trace_xfs_iunlink_update_bucket(tp->t_mountp, agno, bucket_index,
> - old_value, new_agino);
> -
> - /* check if the old agi_unlinked head is as expected */
> - if (old_value != old_agino) {
> + old_agino = be32_to_cpu(agi->agi_unlinked[bucket_index]);
> + if (new_agino == old_agino || cur_agino != old_agino ||
> + !xfs_verify_agino_or_null(mp, agno, old_agino)) {
> xfs_buf_mark_corrupt(agibp);
> return -EFSCORRUPTED;
> }
>
> + trace_xfs_iunlink_update_bucket(mp, agno, bucket_index,
> + old_agino, new_agino);
> +
> agi->agi_unlinked[bucket_index] = cpu_to_be32(new_agino);
> offset = offsetof(struct xfs_agi, agi_unlinked) +
> (sizeof(xfs_agino_t) * bucket_index);
> @@ -2032,44 +2041,25 @@ xfs_iunlink_insert_inode(
> struct xfs_inode *ip)
> {
> struct xfs_mount *mp = tp->t_mountp;
> - struct xfs_agi *agi;
> struct xfs_inode *nip;
> - xfs_agino_t next_agino;
> + xfs_agino_t next_agino = NULLAGINO;
> xfs_agino_t agino = XFS_INO_TO_AGINO(mp, ip->i_ino);
> xfs_agnumber_t agno = XFS_INO_TO_AGNO(mp, ip->i_ino);
> int error;
>
> - agi = agibp->b_addr;
> -
> - /*
> - * We don't need to traverse the on disk unlinked list to find the
> - * previous inode in the list when removing inodes anymore, so we don't
> - * need multiple on-disk lists anymore. Hence we always use bucket 0.
> - * Make sure the pointer isn't garbage and that this inode isn't already
> - * on the list.
> - */
> - next_agino = be32_to_cpu(agi->agi_unlinked[0]);
> - if (next_agino == agino ||
> - !xfs_verify_agino_or_null(mp, agno, next_agino)) {
> - xfs_buf_mark_corrupt(agibp);
> - return -EFSCORRUPTED;
> - }
> -
> nip = list_first_entry_or_null(&agibp->b_pag->pag_ici_unlink_list,
> struct xfs_inode, i_unlink);
> if (nip) {
> - ASSERT(next_agino == XFS_INO_TO_AGINO(mp, nip->i_ino));
>
> /*
> * There is already another inode in the bucket, so point this
> * inode to the current head of the list.
> */
> + next_agino = XFS_INO_TO_AGINO(mp, nip->i_ino);
> error = xfs_iunlink_update_inode(tp, ip, agno, NULLAGINO,
> next_agino);
> if (error)
> return error;
> - } else {
> - ASSERT(next_agino == NULLAGINO);
> }
>
> /* Point the head of the list to point to this inode. */
> @@ -2122,28 +2112,11 @@ xfs_iunlink_remove_inode(
> struct xfs_inode *ip)
> {
> struct xfs_mount *mp = tp->t_mountp;
> - struct xfs_agi *agi;
> xfs_agino_t agino = XFS_INO_TO_AGINO(mp, ip->i_ino);
> xfs_agnumber_t agno = XFS_INO_TO_AGNO(mp, ip->i_ino);
> xfs_agino_t next_agino = NULLAGINO;
> - xfs_agino_t head_agino;
> int error;
>
> - agi = agibp->b_addr;
> -
> - /*
> - * We don't need to traverse the on disk unlinked list to find the
> - * previous inode in the list when removing inodes anymore, so we don't
> - * need multiple on-disk lists anymore. Hence we always use bucket 0.
> - * Make sure the head pointer isn't garbage.
> - */
> - head_agino = be32_to_cpu(agi->agi_unlinked[0]);
> - if (!xfs_verify_agino(mp, agno, head_agino)) {
> - XFS_CORRUPTION_ERROR(__func__, XFS_ERRLEVEL_LOW, mp,
> - agi, sizeof(*agi));
> - return -EFSCORRUPTED;
> - }
> -
> /*
> * Get the next agino in the list. If we are at the end of the list,
> * then the previous inode's i_next_unlinked filed will get cleared.
> @@ -2165,7 +2138,6 @@ xfs_iunlink_remove_inode(
> struct xfs_inode, i_unlink)) {
> struct xfs_inode *pip = list_prev_entry(ip, i_unlink);
>
> - ASSERT(head_agino != agino);
> return xfs_iunlink_update_inode(tp, pip, agno, agino,
> next_agino);
> }
> --
> 2.26.2.761.g0e0b3e54be
>
next prev parent reply other threads:[~2020-08-19 0:31 UTC|newest]
Thread overview: 51+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-12 9:25 [PATCH 00/13] xfs: in memory inode unlink log items Dave Chinner
2020-08-12 9:25 ` [PATCH 01/13] xfs: xfs_iflock is no longer a completion Dave Chinner
2020-08-18 23:44 ` Darrick J. Wong
2020-08-22 7:41 ` Christoph Hellwig
2020-08-12 9:25 ` [PATCH 02/13] xfs: add log item precommit operation Dave Chinner
2020-08-22 9:06 ` Christoph Hellwig
2020-08-12 9:25 ` [PATCH 03/13] xfs: factor the xfs_iunlink functions Dave Chinner
2020-08-18 23:49 ` Darrick J. Wong
2020-08-22 7:45 ` Christoph Hellwig
2020-08-12 9:25 ` [PATCH 04/13] xfs: arrange all unlinked inodes into one list Dave Chinner
2020-08-18 23:59 ` Darrick J. Wong
2020-08-19 0:45 ` Dave Chinner
2020-08-19 0:58 ` Gao Xiang
2020-08-22 9:01 ` Christoph Hellwig
2020-08-23 17:24 ` Gao Xiang
2020-08-24 8:19 ` [RFC PATCH] xfs: use log_incompat feature instead of speculate matching Gao Xiang
2020-08-24 8:34 ` Gao Xiang
2020-08-24 15:08 ` Darrick J. Wong
2020-08-24 15:41 ` Gao Xiang
2020-08-25 10:06 ` [PATCH] " Gao Xiang
2020-08-25 14:54 ` Darrick J. Wong
2020-08-25 15:30 ` Gao Xiang
2020-08-27 7:19 ` Christoph Hellwig
2020-08-12 9:25 ` [PATCH 05/13] xfs: add unlink list pointers to xfs_inode Dave Chinner
2020-08-19 0:02 ` Darrick J. Wong
2020-08-19 0:47 ` Dave Chinner
2020-08-22 9:03 ` Christoph Hellwig
2020-08-25 5:17 ` Dave Chinner
2020-08-27 7:21 ` Christoph Hellwig
2020-08-12 9:25 ` [PATCH 06/13] xfs: replace iunlink backref lookups with list lookups Dave Chinner
2020-08-19 0:13 ` Darrick J. Wong
2020-08-19 0:52 ` Dave Chinner
2020-08-12 9:25 ` [PATCH 07/13] xfs: mapping unlinked inodes is now redundant Dave Chinner
2020-08-19 0:14 ` Darrick J. Wong
2020-08-12 9:25 ` [PATCH 08/13] xfs: updating i_next_unlinked doesn't need to return old value Dave Chinner
2020-08-19 0:19 ` Darrick J. Wong
2020-08-12 9:25 ` [PATCH 09/13] xfs: validate the unlinked list pointer on update Dave Chinner
2020-08-19 0:23 ` Darrick J. Wong
2020-08-12 9:25 ` [PATCH 10/13] xfs: re-order AGI updates in unlink list updates Dave Chinner
2020-08-19 0:29 ` Darrick J. Wong [this message]
2020-08-19 1:01 ` Dave Chinner
2020-08-12 9:25 ` [PATCH 11/13] xfs: combine iunlink inode update functions Dave Chinner
2020-08-19 0:30 ` Darrick J. Wong
2020-08-12 9:25 ` [PATCH 12/13] xfs: add in-memory iunlink log item Dave Chinner
2020-08-19 0:35 ` Darrick J. Wong
2020-08-12 9:25 ` [PATCH 13/13] xfs: reorder iunlink remove operation in xfs_ifree Dave Chinner
2020-08-12 11:12 ` Gao Xiang
2020-08-19 0:45 ` Darrick J. Wong
2020-08-18 18:17 ` [PATCH 00/13] xfs: in memory inode unlink log items Darrick J. Wong
2020-08-18 20:01 ` Gao Xiang
2020-08-18 21:42 ` 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=20200819002948.GX6096@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.