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

["Darrick J. Wong" <darrick.wong@oracle.com>]

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.

> 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...

...oh well I should probably go complain to the new series, not this
one.

--D


> > 
> > --D
> > 
> > >  	sb->s_fs_info = mp;
> > >  
> > >  	error = xfs_parseargs(mp, (char *)data);
> > > 
>