Re: [PATCH v2] xfs: Fix deadlock between AGI and AGF when target_ip exists in xfs_rename()

[Dave Chinner <david@fromorbit.com>]

On Wed, Nov 06, 2019 at 07:46:12AM -0800, Darrick J. Wong wrote:
> On Wed, Nov 06, 2019 at 07:49:32AM -0500, Brian Foster wrote:
> > > >  /*
> > > > + * 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. :)

So as a structural question: should this be folded into
xfs_dir_canenter(), which is the function used to check if the
directory modification can go ahead without allocating blocks....

This seems very much like it is a "do we need to allocate blocks
during the directory modification?" sort of question being asked
here...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com