| Message ID | 157259466607.28278.4456308072088112584.stgit@fedora-28 (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | xfs: mount API patch series | expand |
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.) --D > sb->s_fs_info = mp; > > error = xfs_parseargs(mp, (char *)data); >
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. 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! > > --D > > > sb->s_fs_info = mp; > > > > error = xfs_parseargs(mp, (char *)data); > >
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); > > > >
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
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; 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; sb->s_fs_info = mp; error = xfs_parseargs(mp, (char *)data);