Received-SPF: pass (google.com: domain of linux-kernel+bounces-171601-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Tue, 7 May 2024 16:43:19 +0200
From: Petr Vorel <pvorel@suse.cz>
To: Brian Foster <bfoster@redhat.com>
Cc: linux-bcachefs@vger.kernel.org,
	Kent Overstreet <kent.overstreet@linux.dev>, Su Yue <l@damenly.org>,
	Coly Li <colyli@suse.de>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/1] bcachefs: Move BCACHEFS_STATFS_MAGIC to UAPI magic.h
Message-ID: <20240507144319.GC138935@pevik>
Reply-To: Petr Vorel <pvorel@suse.cz>
References: <20240507111124.118520-1-pvorel@suse.cz>
 <Zjot36J9negcVlfh@bfoster>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Zjot36J9negcVlfh@bfoster>

Hi Brian,

> On Tue, May 07, 2024 at 01:11:24PM +0200, Petr Vorel wrote:
> > There are other bcachefs magic definitions: BCACHE_MAGIC, BCHFS_MAGIC,
> > which use UUID_INIT() and are used only in libbcachefs. Therefore move
> > to <linux/magic.h> only BCACHEFS_STATFS_MAGIC, which can be used outside
> > of libbcachefs for f_type field in struct statfs in statfs() or fstatfs().

> > Keeping non-standard name BCACHEFS_STATFS_MAGIC instead of renaming it
> > to more generic BCACHEFS_MAGIC to not confuse with the other bcachefs
> > definitions.


> Perhaps it would be better to use the standard naming for the global
> header and let the bcachefs subsystem catch up with better names for
> internal defs? Something like BCACHEFS_SUPER_MAGIC also seems like it
> would be generally consistent, FWIW.

I was also thinking about this. I'll send v2.

> > Suggested-by: Su Yue <l@damenly.org>
> > Signed-off-by: Petr Vorel <pvorel@suse.cz>
> > ---
> >  fs/bcachefs/bcachefs_format.h | 2 --
> >  fs/bcachefs/fs.c              | 1 +
> >  include/uapi/linux/magic.h    | 2 ++
> >  3 files changed, 3 insertions(+), 2 deletions(-)

> > diff --git a/fs/bcachefs/bcachefs_format.h b/fs/bcachefs/bcachefs_format.h
> > index f7fbfccd2b1e..52e03f13b780 100644
> > --- a/fs/bcachefs/bcachefs_format.h
> > +++ b/fs/bcachefs/bcachefs_format.h
> > @@ -1275,8 +1275,6 @@ enum bch_compression_opts {
> >  	UUID_INIT(0xc68573f6, 0x66ce, 0x90a9,				\
> >  		  0xd9, 0x6a, 0x60, 0xcf, 0x80, 0x3d, 0xf7, 0xef)

> > -#define BCACHEFS_STATFS_MAGIC		0xca451a4e
> > -
> >  #define JSET_MAGIC		__cpu_to_le64(0x245235c1a3625032ULL)
> >  #define BSET_MAGIC		__cpu_to_le64(0x90135c78b99e07f5ULL)

> > diff --git a/fs/bcachefs/fs.c b/fs/bcachefs/fs.c
> > index fce690007edf..1c856ae2b0e2 100644
> > --- a/fs/bcachefs/fs.c
> > +++ b/fs/bcachefs/fs.c
> > @@ -27,6 +27,7 @@
> >  #include "super.h"
> >  #include "xattr.h"

> > +#include <uapi/linux/magic.h>
> >  #include <linux/aio.h>
> >  #include <linux/backing-dev.h>
> >  #include <linux/exportfs.h>
> > diff --git a/include/uapi/linux/magic.h b/include/uapi/linux/magic.h
> > index 1b40a968ba91..45c8f4916167 100644
> > --- a/include/uapi/linux/magic.h
> > +++ b/include/uapi/linux/magic.h
> > @@ -103,4 +103,6 @@
> >  #define SECRETMEM_MAGIC		0x5345434d	/* "SECM" */
> >  #define PID_FS_MAGIC		0x50494446	/* "PIDF" */

> > +#define BCACHEFS_STATFS_MAGIC		0xca451a4e
> > +

> Is there intended to be at least some logical organization to this file?
> It kind of looks like it, but maybe not as a rule. Personally, I'd
> probably stick this somewhere in the first chunk of definitions where
> the other major local/block filesystems are, but just a nit.

I was asking myself, but did not see any rule :). But sure, I can add it below
FUSE_SUPER_MAGIC (the end of the first chunk).

Thanks for your review!

Kind regards,
Petr

> Brian

> >  #endif /* __LINUX_MAGIC_H__ */
> > -- 
> > 2.43.0