Re: [PATCH 4/6] composefs: Add filesystem implementation

[Alexander Larsson <alexl@redhat.com>]

On Fri, 2023-01-06 at 07:18 -0500, Brian Masney wrote:
> On Mon, Nov 28, 2022 at 12:17:12PM +0100, Alexander Larsson wrote:
> > This is the basic inode and filesystem implementation.
> > 
> > Signed-off-by: Alexander Larsson <alexl@redhat.com>
> > Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
> 
> Note: I'm looking at this from the VFS viewpoint since I haven't done
> anything in this subsystem. Just looking for some generic
> suggestions.
> 
> > 
> > +
> > +struct cfs_inode {
> > +       /* must be first for clear in cfs_alloc_inode to work */
> > +       struct inode vfs_inode;
> 
> [ snip ]
> 
> > +static struct inode *cfs_alloc_inode(struct super_block *sb)
> > +{
> > +       struct cfs_inode *cino =
> > +               alloc_inode_sb(sb, cfs_inode_cachep, GFP_KERNEL);
> > +
> > +       if (!cino)
> > +               return NULL;
> > +
> > +       memset((u8 *)cino + sizeof(struct inode), 0,
> > +              sizeof(struct cfs_inode) - sizeof(struct inode));
> 
> Why not use container_of() to look up the vfs_inode and then you can
> get
> rid of the restriction of this being first. This may also break with
> structure randomization turned on.

This is not clearing vfs_inode though, it is clearing everything after
vfs_inode, because we're using an allocation cache for the vfs_inode
part. I don't see how container_of can help us here?

Also, surely structure randomization won't change the offset of the
first element of the struct? That will break all sorts of "derived
object" usecases.

> 
> > +static inline struct cfs_inode *CFS_I(struct inode *inode)
> 
> CFS_I in upper case doesn't match naming conventions in the rest of
> the kernel.
> 

It is similar to VFS_I, XFS_I, BTRFS_I, EXT4_I, etc

> > +static unsigned int cfs_split_basedirs(char *str)
> > +{
> > +       unsigned int ctr = 1;
> > +       char *s, *d;
> > +
> > +       for (s = d = str;; s++, d++) {
> > +               if (*s == '\\') {
> > +                       s++;
> > +               } else if (*s == ':') {
> > +                       *d = '\0';
> > +                       ctr++;
> > +                       continue;
> > +               }
> > +               *d = *s;
> > +               if (!*s)
> > +                       break;
> > +       }
> > +       return ctr;
> > +}
> 
> To expand on the comment, this is ovl_split_lowerdirs in
> fs/overlayfs/super.c. It'd be nice if there was a common place where
> this could go.
> 

It would be nice, but its not like a huge amount of code duplication,
as it is rather specialized.

> 
> 

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
=-=-=
 Alexander Larsson                                            Red Hat,
Inc 
       alexl@redhat.com            alexander.larsson@gmail.com 
He's a superhumanly strong neurotic barbarian with no name. She's a
blind 
cigar-chomping bounty hunter from a different time and place. They
fight 
crime!