Re: [PATCH v8 12/16] xfs: dont set sb in xfs_mount_alloc()

[Ian Kent <raven@themaw.net>]

On Mon, 2019-11-04 at 13:12 -0800, Darrick J. Wong wrote:
> On Sat, Nov 02, 2019 at 12:41:39PM +0800, Ian Kent wrote:
> > On Fri, 2019-11-01 at 13:15 -0700, Darrick J. Wong wrote:
> > > On Fri, Nov 01, 2019 at 03:51:06PM +0800, Ian Kent wrote:
> > > > When changing to use the new mount api the super block won't be
> > > > available when the xfs_mount struct is allocated so move
> > > > setting
> > > > the
> > > > super block in xfs_mount to xfs_fs_fill_super().
> > > > 
> > > > Signed-off-by: Ian Kent <raven@themaw.net>
> > > > Reviewed-by: Brian Foster <bfoster@redhat.com>
> > > > ---
> > > >  fs/xfs/xfs_super.c |    7 +++----
> > > >  1 file changed, 3 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > > > index 4b570ba3456a..62dfc678c415 100644
> > > > --- a/fs/xfs/xfs_super.c
> > > > +++ b/fs/xfs/xfs_super.c
> > > > @@ -1560,8 +1560,7 @@ xfs_destroy_percpu_counters(
> > > >  }
> > > >  
> > > >  static struct xfs_mount *
> > > > -xfs_mount_alloc(
> > > > -	struct super_block	*sb)
> > > > +xfs_mount_alloc(void)
> > > >  {
> > > >  	struct xfs_mount	*mp;
> > > >  
> > > > @@ -1569,7 +1568,6 @@ xfs_mount_alloc(
> > > >  	if (!mp)
> > > >  		return NULL;
> > > >  
> > > > -	mp->m_super = sb;
> > > 
> > > Just out of curiosity, is there any place where we need m_super
> > > in
> > > between here...
> > > 
> > > >  	spin_lock_init(&mp->m_sb_lock);
> > > >  	spin_lock_init(&mp->m_agirotor_lock);
> > > >  	INIT_RADIX_TREE(&mp->m_perag_tree, GFP_ATOMIC);
> > > > @@ -1605,9 +1603,10 @@ xfs_fs_fill_super(
> > > >  	 * allocate mp and do all low-level struct
> > > > initializations
> > > > before we
> > > >  	 * attach it to the super
> > > >  	 */
> > > > -	mp = xfs_mount_alloc(sb);
> > > > +	mp = xfs_mount_alloc();
> > > >  	if (!mp)
> > > >  		goto out;
> > > > +	mp->m_super = sb;
> > > 
> > > ...and here?  For example, logging errors?  AFAICT the only thing
> > > that
> > > goes on between these two points is option parsing, right?  (And
> > > the
> > > parsing has its own prefixed logging, etc.)
> > 
> > Yes, only option parsing is going on between these two points.
> > 
> > And, for now, the error reporting is caught by the VFS.
> > 
> > There is one location in xfs_fc_parse_param() where an xfs log
> > message could be emitted although it's never reached (because of
> > the return if the fs_parse() call fails).
> > 
> > If log messages were issued in between these two points the
> > consequence
> > is a missing block device name in the message. You remember, a
> > check on
> > mp->m_super was added to __xfs_printk() to cover this case when
> > struct
> > xfs_mount field m_fsname was eliminated.
> 
> It's true that (AFAICT) this is the only place where xfs might need
> mp->m_super but it doesn't yet have one, but you'd agree that this is
> a
> significant change to the scoping rules, right?  In the past there
> was
> never a place in xfs where we'd have to check mp->m_super == NULL,
> but
> now we have to keep that possibility in mind, at least for any
> function
> that can be called before get_tree_bdev.

Yes, it is, but that is forced on file systems by the VFS.

I'm not certain this design is needed to support the new
fsopen()/fsconfig()/fsmount() system calls though I think it is
due to the way in which they are called.

For example I don't think the VFS has enough information to obtain
the super block at the time fsconfig() is being called so it must
be deferred until fsmount() and fsconfig() is about parameter parsing.

> 
> > This potential lack of device name in log messages is a problem I
> > can't
> > fix because the block device isn't obtained until after parameter
> > parsing, just before the super block is acquired. Changing that in
> > the
> > VFS would be quite significant so I'm stuck!
> 
> Um, we used to obtain the block device and the superblock before we
> started option parsing.  I guess the worst that happens is that
> anything
> trying to dereference mp->m_super is just going to crash...

Yes, but the xfs logging is probably the most important thing that
would be needed prior to calling get_tree_bdev() and that's currently
ok except for the missing block device name.

There may be ways to change this but I'm pretty sure they would cause
fairly significant challenges for the overall design and support of
those new system calls and that's one of the key elements of the new
mount-api.

Ian