Re: move all statfs f_type magic numbers to linux/magic.h

[Dave Chinner <david@fromorbit.com>]

On Tue, Mar 04, 2014 at 01:37:59PM +0100, Karel Zak wrote:
> On Tue, Mar 04, 2014 at 09:58:07PM +1100, Dave Chinner wrote:
> > On Tue, Mar 04, 2014 at 11:00:36AM +0100, Karel Zak wrote:
> > > The file <linux/magic.h> is incomplete and some magic numbers that kernel
> > > returns by statfs f_type to userspace are not defined there. This situation
> > > makes statfs f_type usage in userpace painful as you have to "ifndef-define"
> > > many magic numbers in userspace code.
> > > 
> > > All the patches are trivial and just move *_SUPER_MAGIC stuff from 
> > > filesystem specific files to the generic <linux/magic.h>.
> > 
> > IMO, this is the wrong way to fix the statfs magic numbers into the
> > user API. You're changing the code to define magic numbers that are
> > on disk to be defined - and fixed permanently - by the statfs
> > syscall API.
> 
>  Well, my idea was to follow the current kernel manner :-) and add the 
>  missing magic numbers to the API.
> 
>  I agree that linux/magic.h is mess, and if there is call for something 
>  better than I can send v2 with API cleanup.
> 
> > We need to be able to change the on-disk magic numbers whenever and
> > however we want without affecting userspace
> 
>  It's still possible, just define a new FOO_SUPER_MAGIC2 in fs code 
>  and keep linux/magic.h unchanged. 

Which points out exactly why the coupling this patch set introduces
is wrong.

>  And not in all cases *_SUPER_MAGIC are used for on-disk stuff, very
>  often it's magic number for s_magic and f_type only.

Sure, those cases are fine. But it's not ok to move an on-disk
format definition into the userspace API.

> > If you want all the magic numbers that filesystems expose to
> > userspace in statfs to be defined in one place, then you need to
> > create the definitions for only that purpose that match what is
> 
>  Yes, I thought about it, but I was not sure if such change will be
>  accepted.
> 
> > currently documented in the statfs() man page and use those in the
> > filesystem statfs implementation rather than the on-disk superblock
> > magic number definitions.
> > 
> > e.g. include/uapi/linux/statfs_magic.h:
> > 
> > #define __STATFS_ADFS_SUPER_MAGIC	0xadf5
> > #define __STATFS_AFFS_SUPER_MAGIC	0xADFF
> > ...
> 
>  Now filesystems usually share statfs f_type with superblock s_magic. 
>  It would be probably good enough to use something like
> 
>  include/uapi/linux/statfs_magic.h:
> 
>     #define __STATFS_FOO_SUPER_MAGIC 0xadf5
> 
>  fs/foo/foo_sb.h:
> 
>     #define FOO_SUPER_MAGIC     __STATFS_FOO_SUPER_MAGIC
> 
>  to avoid multiple definitions of the same numbers.

That has exactly the same problem as this patchset - you're
coupling the on-disk format definition to an unrelated userspace API
definition.

We shouldn't have a single shared definition of magic numbers for
statfs and on-disk formats. They are independent, have different
rules determining how they can be changed and have completely
different purposes in life. Hence they should be defined separately,
even if they are initially defined to the same value....

>  and what about the old linux/magic.h? I guess it's necessary
>  to keep it for backward compatibility.

It's part of the UAPI - it has to remain there forever now....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com