Re: [PATCH v4 00/17] xfs: mount API patch series

[Ian Kent <raven@themaw.net>]

On Mon, 2019-10-07 at 17:35 -0700, Darrick J. Wong wrote:
> On Tue, Oct 08, 2019 at 08:13:57AM +0800, Ian Kent wrote:
> > On Mon, 2019-10-07 at 07:52 -0400, Brian Foster wrote:
> > > On Thu, Oct 03, 2019 at 06:25:18PM +0800, Ian Kent wrote:
> > > > This patch series add support to xfs for the new kernel mount
> > > > API
> > > > as described in the LWN article at 
> > > > https://lwn.net/Articles/780267/
> > > > .
> > > > 
> > > > In the article there's a lengthy description of the reasons for
> > > > adopting the API and problems expected to be resolved by using
> > > > it.
> > > > 
> > > > The series has been applied to the repository located at
> > > > git://git.kernel.org/pub/scm/fs/xfs/xfs-linux.git, and built
> > > > and
> > > > some simple tests run on it along with the generic xfstests.
> > > > 
> > > 
> > > I'm not sure that we have any focused mount option testing in
> > > fstests.
> > > It looks like we have various remount tests and such to cover
> > > corner
> > > cases and/or specific bugs, but nothing to make sure various
> > > options
> > > continue to work or otherwise fail gracefully. Do you have any
> > > plans
> > > to
> > > add such a test to help verify this work?
> > 
> > Darrick was concerned about that.
> > 
> > Some sort of xfstest is needed in order to be able to merge this
> > so he has some confidence that it won't break things.
> > 
> > I volunteered to do have a go at writing a test.
> > 
> > I've given that some thought and done an initial survey of xfstests
> > but it's still new to me so I'm not sure how this will end up.
> > 
> > Darrick thought it would need a generic test to test VFS options
> > and one in xfs for the xfs specific options.
> > 
> > At this point I'm thinking I'll have a go at adding an xfs specific
> > options test but, while I can find out what the optionsare and what
> > validation they use, there's a lot about some of the xfs options
> > I don't fully understand so I don't know what a sensible test might
> > be.
> 
> Hm, that's evidence of inadequate documentation.  If you can't figure
> out what would be sensible tests for a particular mount option from
> xfs(5) then we have work to do. :)

Maybe, I have looked at xfs(5) but haven't yet started trying to
work out what I need to do so we will see how that goes.

> 
> So if you can't come up with something that seems 'reasonable' to
> test,
> I suggest random gibberish(!) and send the outcome of those
> iterations
> to the list to see what kinds of arguments you can stir up.  Since
> we're
> only interested in testing the mounting code here, you can declare
> victory if the fs mounts, never mind if the option actually has any
> effect on fs operations.  That kind of functional testing should be
> in
> separate tests anyway.

I thought your suggestion of minimum, maximum and out of range for
options that have a range is good. There's also the individual
options which should be straight forward.

But there's a range of other options that sound like they aren't
straight forward.

For example, IIRC, I can give inode64 or inode32 on mount regardless
of (presumably) the on-disk inode size which seemed odd to me.

But of course the file system isn't mounted yet so the options
parsing won't know this at the time. I supposed that would be
handled later, probably with some sort of warning to the log.

> 
> One advantage that you probably have over us is that our
> understanding
> of the mount options and associated behavior is based on a lot of
> experience working in the code base, whereas most everyone else's is
> based entirely on whatever's in the manpage.  It's helpful to have
> someone hold us to our words every now and then.

Indeed, I think this will be a useful exercise for xfs and myself.

> 
> (This is going to get interesting when we get to mount options whose
> validity changes depending on mkfs parameters, etc.)

Second pass of writing the test will need input on that.

Perhaps (but probably not yet so I don't make implicit assumptions)
someone could come up with a list of common mkfs vs needed mount
options for the more sophisticated tests once I get to them.

Ian
> 
> --D
> 
> > > Brian
> > > 
> > > > Other things that continue to cause me concern:
> > > > 
> > > > - Message logging.
> > > >   There is error logging done in the VFS by the mount-api code,
> > > > some
> > > >   is VFS specific while some is file system specific. This can
> > > > lead
> > > >   to duplicated and sometimes inconsistent logging.
> > > > 
> > > >   The mount-api feature of saving error message text to the
> > > > mount
> > > >   context for later retrieval by fsopen()/fsconfig()/fsmount()
> > > > users
> > > >   is the reason the mount-api log macros are present. But, at
> > > > the
> > > >   moment (last time I looked), these macros will either log the
> > > >   error message or save it to the mount context. There's not
> > > > yet
> > > >   a way to modify this behaviour so it which can lead to
> > > > messages,
> > > >   possibly needed for debug purposes, not being sent to the
> > > > kernel
> > > >   log. There's also the pr_xxx() log functions (not a problem
> > > > for
> > > >   xfs AFAICS) that aren't aware of the mount context at all.
> > > > 
> > > >   In the xfs patches I've used the same method that is used in
> > > >   gfs2 and was suggested by Al Viro (essentially return the
> > > > error
> > > >   if fs_parse() returns one) except that I've also not used the
> > > >   mount api log macros to minimise the possibility of lost log
> > > >   messages.
> > > > 
> > > >   This isn't the best so follow up patches for RFC (with a
> > > >   slightly wider audience) will be needed to try and improve
> > > >   this aspect of the mount api.
> > > > 
> > > > Changes for v4:
> > > > - changed xfs_fill_super() cleanup back to what it was in v2,
> > > > until
> > > >   I can work out what's causing the problem had previously seen
> > > > (I
> > > > can't
> > > >   reproduce it myself), since it looks like it was right from
> > > > the
> > > > start.
> > > > - use get_tree_bdev() instead of vfs_get_block_super() in
> > > > xfs_get_tree()
> > > >   as requested by Al Viro.
> > > > - removed redundant initialisation in xfs_fs_fill_super().
> > > > - fix long line in xfs_validate_params().
> > > > - no need to validate if parameter parsing fails, just return
> > > > the
> > > > error.
> > > > - summarise reconfigure comment about option handling, transfer
> > > > bulk
> > > >   of comment to commit log message.
> > > > - use minimal change in xfs_parse_param(), deffer discussion of
> > > > mount
> > > >   api logging improvements until later and with a wider
> > > > audience.
> > > > 
> > > > Changes for v3:
> > > > - fix struct xfs_fs_context initialisation in xfs_parseargs().
> > > > - move call to xfs_validate_params() below label "done".
> > > > - if allocation of xfs_mount fails return ENOMEM immediately.
> > > > - remove erroneous kfree(mp) in xfs_fill_super().
> > > > - move the removal of xfs_fs_remount() and
> > > > xfs_test_remount_options()
> > > >   to the switch to mount api patch.
> > > > - retain original usage of distinct <option>, no<option> usage.
> > > > - fix line length and a white space problem in xfs_parseargs().
> > > > - defer introduction of struct fs_context_operations until
> > > > mount
> > > >   api implementation.
> > > > - don't use a new line for the void parameter of
> > > > xfs_mount_alloc().
> > > > - check for -ENOPARAM in xfs_parse_param() to report invalid
> > > > options
> > > >   using the options switch (to avoid double entry log
> > > > messages).
> > > > - remove obsolete mount option biosize.
> > > > - try and make comment in xfs_fc_free() more understandable.
> > > > 
> > > > Changes for v2:
> > > > - changed .name to uppercase in fs_parameter_description to
> > > > ensure
> > > >   consistent error log messages between the vfs parser and the
> > > > xfs
> > > >   parameter parser.
> > > > - clarify comment above xfs_parse_param() about when possibly
> > > >   allocated mp->m_logname or mp->m_rtname are freed.
> > > > - factor out xfs_remount_rw() and xfs_remount_ro()
> > > > from  xfs_remount().
> > > > - changed xfs_mount_alloc() to not set super block in xfs_mount
> > > > so
> > > > it
> > > >   can be re-used when switching to the mount-api.
> > > > - fixed don't check for NULL when calling kfree() in
> > > > xfs_fc_free().
> > > > - refactored xfs_parseargs() in an attempt to highlight the
> > > > code
> > > >   that actually changes in converting to use the new mount api.
> > > > - dropped xfs-mount-api-rename-xfs_fill_super.patch, it didn't
> > > > seem
> > > >   necessary.
> > > > - move comment about remount difficulties above
> > > > xfs_reconfigure()
> > > >   and increase line length to try and make the comment
> > > > manageable.
> > > > 
> > > > Al Viro has sent a pull request to Linus for the patch
> > > > containing
> > > > get_tree_bdev() recently and I think there's a small problem
> > > > with
> > > > that patch too so there will be conflicts with merging this
> > > > series
> > > > without dropping the first two patches of the series.
> > > > 
> > > > ---
> > > > 
> > > > David Howells (1):
> > > >       vfs: Create fs_context-aware mount_bdev() replacement
> > > > 
> > > > Ian Kent (16):
> > > >       vfs: add missing blkdev_put() in get_tree_bdev()
> > > >       xfs: remove very old mount option
> > > >       xfs: mount-api - add fs parameter description
> > > >       xfs: mount-api - refactor suffix_kstrtoint()
> > > >       xfs: mount-api - refactor xfs_parseags()
> > > >       xfs: mount-api - make xfs_parse_param() take context
> > > > .parse_param() args
> > > >       xfs: mount-api - move xfs_parseargs() validation to a
> > > > helper
> > > >       xfs: mount-api - refactor xfs_fs_fill_super()
> > > >       xfs: mount-api - add xfs_get_tree()
> > > >       xfs: mount-api - add xfs_remount_rw() helper
> > > >       xfs: mount-api - add xfs_remount_ro() helper
> > > >       xfs: mount api - add xfs_reconfigure()
> > > >       xfs: mount-api - add xfs_fc_free()
> > > >       xfs: mount-api - dont set sb in xfs_mount_alloc()
> > > >       xfs: mount-api - switch to new mount-api
> > > >       xfs: mount-api - remove remaining legacy mount code
> > > > 
> > > > 
> > > >  fs/super.c                 |   97 +++++
> > > >  fs/xfs/xfs_super.c         |  939 +++++++++++++++++++++++-----
> > > > ----
> > > > ------------
> > > >  include/linux/fs_context.h |    5 
> > > >  3 files changed, 600 insertions(+), 441 deletions(-)
> > > > 
> > > > --
> > > > Ian