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@xxxxxxxxxx>
> > > Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>
> > > ---
> > > 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);
> > >
>
