On Wed, Nov 06, 2019 at 07:49:32AM -0500, Brian Foster wrote:
> On Tue, Nov 05, 2019 at 08:56:30PM -0800, Darrick J. Wong wrote:
> > On Tue, Nov 05, 2019 at 05:52:12PM +0800, kaixuxia wrote:
> > > When target_ip exists in xfs_rename(), the xfs_dir_replace() call may
> > > need to hold the AGF lock to allocate more blocks, and then invoking
> > > the xfs_droplink() call to hold AGI lock to drop target_ip onto the
> > > unlinked list, so we get the lock order AGF->AGI. This would break the
> > > ordering constraint on AGI and AGF locking - inode allocation locks
> > > the AGI, then can allocate a new extent for new inodes, locking the
> > > AGF after the AGI.
> > >
> > > In this patch we check whether the replace operation need more
> > > blocks firstly. If so, acquire the agi lock firstly to preserve
> > > locking order(AGI/AGF). Actually, the locking order problem only
> > > occurs when we are locking the AGI/AGF of the same AG. For multiple
> > > AGs the AGI lock will be released after the transaction committed.
> > >
> > > Signed-off-by: kaixuxia <kaixuxia@xxxxxxxxxxx>
> > > ---
> > > Changes in v2:
> > > - Add xfs_dir2_sf_replace_needblock() helper in
> > > xfs_dir2_sf.c.
> > >
> > > fs/xfs/libxfs/xfs_dir2.c | 23 +++++++++++++++++++++++
> > > fs/xfs/libxfs/xfs_dir2.h | 2 ++
> > > fs/xfs/libxfs/xfs_dir2_priv.h | 2 ++
> > > fs/xfs/libxfs/xfs_dir2_sf.c | 24 ++++++++++++++++++++++++
> > > fs/xfs/xfs_inode.c | 14 ++++++++++++++
> > > 5 files changed, 65 insertions(+)
> > >
> > > diff --git a/fs/xfs/libxfs/xfs_dir2.c b/fs/xfs/libxfs/xfs_dir2.c
> > > index 867c5de..1917990 100644
> > > --- a/fs/xfs/libxfs/xfs_dir2.c
> > > +++ b/fs/xfs/libxfs/xfs_dir2.c
> > > @@ -463,6 +463,29 @@
> > > }
> > >
> > > /*
> > > + * Check whether the replace operation need more blocks. Ignore
> > > + * the parameters check since the real replace() call below will
> > > + * do that.
> > > + */
> > > +bool
> > > +xfs_dir_replace_needblock(
> >
> > xfs_dir2, to be consistent.
> >
> > > + struct xfs_inode *dp,
> > > + xfs_ino_t inum)
> >
> > If you passed the inode pointer (instead of ip->i_ino) here then you
> > don't need to revalidate the inode number.
> >
> > > +{
> > > + int rval;
> > > +
> > > + rval = xfs_dir_ino_validate(dp->i_mount, inum);
> > > + if (rval)
> > > + return false;
> > > +
> > > + /*
> > > + * Only convert the shortform directory to block form maybe
> > > + * need more blocks.
> > > + */
> > > + return xfs_dir2_sf_replace_needblock(dp, inum);
> >
> > if (dp->i_d.di_format != XFS_DINODE_FMT_LOCAL)
> > return xfs_dir2_sf_replace_needblock(...);
> >
> > Also, do other directories formats need extra blocks allocated?
> >
> > > +}
> > > +
> > > +/*
> > > * Replace the inode number of a directory entry.
> > > */
> > > int
> > > diff --git a/fs/xfs/libxfs/xfs_dir2.h b/fs/xfs/libxfs/xfs_dir2.h
> > > index f542447..e436c14 100644
> > > --- a/fs/xfs/libxfs/xfs_dir2.h
> > > +++ b/fs/xfs/libxfs/xfs_dir2.h
> > > @@ -124,6 +124,8 @@ extern int xfs_dir_lookup(struct xfs_trans *tp, struct xfs_inode *dp,
> > > extern int xfs_dir_removename(struct xfs_trans *tp, struct xfs_inode *dp,
> > > struct xfs_name *name, xfs_ino_t ino,
> > > xfs_extlen_t tot);
> > > +extern bool xfs_dir_replace_needblock(struct xfs_inode *dp,
> > > + xfs_ino_t inum);
> > > extern int xfs_dir_replace(struct xfs_trans *tp, struct xfs_inode *dp,
> > > struct xfs_name *name, xfs_ino_t inum,
> > > xfs_extlen_t tot);
> > > diff --git a/fs/xfs/libxfs/xfs_dir2_priv.h b/fs/xfs/libxfs/xfs_dir2_priv.h
> > > index 59f9fb2..002103f 100644
> > > --- a/fs/xfs/libxfs/xfs_dir2_priv.h
> > > +++ b/fs/xfs/libxfs/xfs_dir2_priv.h
> > > @@ -116,6 +116,8 @@ extern int xfs_dir2_block_to_sf(struct xfs_da_args *args, struct xfs_buf *bp,
> > > extern int xfs_dir2_sf_create(struct xfs_da_args *args, xfs_ino_t pino);
> > > extern int xfs_dir2_sf_lookup(struct xfs_da_args *args);
> > > extern int xfs_dir2_sf_removename(struct xfs_da_args *args);
> > > +extern bool xfs_dir2_sf_replace_needblock(struct xfs_inode *dp,
> > > + xfs_ino_t inum);
> > > extern int xfs_dir2_sf_replace(struct xfs_da_args *args);
> > > extern xfs_failaddr_t xfs_dir2_sf_verify(struct xfs_inode *ip);
> > >
> > > diff --git a/fs/xfs/libxfs/xfs_dir2_sf.c b/fs/xfs/libxfs/xfs_dir2_sf.c
> > > index 85f14fc..0906f91 100644
> > > --- a/fs/xfs/libxfs/xfs_dir2_sf.c
> > > +++ b/fs/xfs/libxfs/xfs_dir2_sf.c
> > > @@ -945,6 +945,30 @@ static int xfs_dir2_sf_addname_pick(xfs_da_args_t *args, int objchange,
> > > }
> > >
> > > /*
> > > + * Check whether the replace operation need more blocks.
> > > + */
> > > +bool
> > > +xfs_dir2_sf_replace_needblock(
> >
> > Urgggh. This is a predicate that we only ever call from xfs_rename(),
> > right? And it addresses a particular quirk of the locking when the
> > caller wants us to rename on top of an existing entry and drop the link
> > count of the old inode, right? So why can't this just be a predicate in
> > xfs_inode.c ? Nobody else needs to know this particular piece of
> > information, AFAICT.
> >
> > (Apologies, for Brian and I clearly aren't on the same page about
> > that...)
> >
>
> Hmm.. the crux of my feedback on the previous version was simply that if
> we wanted to take this approach of pulling up lower level dir logic into
> the higher level rename code, to simply factor out the existing checks
> down in the dir replace code that currently trigger a format conversion,
> and use that new helper in both places. That doesn't appear to be what
> this patch does, and I'm not sure why there are now two new helpers that
> each only have one caller instead of one new helper with two callers...
Aha, got it. I'd wondered if that had been your intent. :)
--D
> Brian
>
> > > + struct xfs_inode *dp,
> > > + xfs_ino_t inum)
> > > +{
> > > + int newsize;
> > > + xfs_dir2_sf_hdr_t *sfp;
> > > +
> > > + if (dp->i_d.di_format != XFS_DINODE_FMT_LOCAL)
> > > + return false;
> >
> > This check should be used up in xfs_dir2_replace_needblock() to decide
> > if we're calling xfs_dir2_sf_replace_needblock(), or just returning
> > false.
> >
> > > +
> > > + sfp = (xfs_dir2_sf_hdr_t *)dp->i_df.if_u1.if_data;
> > > + newsize = dp->i_df.if_bytes + (sfp->count + 1) * XFS_INO64_DIFF;
> > > +
> > > + if (inum > XFS_DIR2_MAX_SHORT_INUM &&
> > > + sfp->i8count == 0 && newsize > XFS_IFORK_DSIZE(dp))
> > > + return true;
> > > + else
> > > + return false;
> >
> > return inum > XFS_DIR2_MAX_SHORT_INUM && (all the rest of that);
> >
> > > +}
> > > +
> > > +/*
> > > * Replace the inode number of an entry in a shortform directory.
> > > */
> > > int /* error */
> > > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > > index 18f4b26..c239070 100644
> > > --- a/fs/xfs/xfs_inode.c
> > > +++ b/fs/xfs/xfs_inode.c
> > > @@ -3196,6 +3196,7 @@ struct xfs_iunlink {
> > > struct xfs_trans *tp;
> > > struct xfs_inode *wip = NULL; /* whiteout inode */
> > > struct xfs_inode *inodes[__XFS_SORT_INODES];
> > > + struct xfs_buf *agibp;
> > > int num_inodes = __XFS_SORT_INODES;
> > > bool new_parent = (src_dp != target_dp);
> > > bool src_is_directory = S_ISDIR(VFS_I(src_ip)->i_mode);
> > > @@ -3361,6 +3362,19 @@ struct xfs_iunlink {
> > > * In case there is already an entry with the same
> > > * name at the destination directory, remove it first.
> > > */
> > > +
> > > + /*
> > > + * Check whether the replace operation need more blocks.
> > > + * If so, acquire the agi lock firstly to preserve locking
> >
> > "first"
> >
> > > + * order(AGI/AGF).
> >
> > Nit: space between "order" and "(AGI/AGF)".
> > > + */
> > > + if (xfs_dir_replace_needblock(target_dp, src_ip->i_ino)) {
> > > + error = xfs_read_agi(mp, tp,
> > > + XFS_INO_TO_AGNO(mp, target_ip->i_ino), &agibp);
> >
> > Overly long line here.
> >
> > --D
> >
> > > + if (error)
> > > + goto out_trans_cancel;
> > > + }
> > > +
> > > error = xfs_dir_replace(tp, target_dp, target_name,
> > > src_ip->i_ino, spaceres);
> > > if (error)
> > > --
> > > 1.8.3.1
> > >
>
