Re: [PATCH 4/4] xfs: V5 filesystem format support

[Jan Kara]

On Tue 12-05-15 08:23:40, Andrei Borzenkov wrote:
...

> > @@ -123,17 +144,6 @@ struct grub_xfs_inode
> >    grub_uint16_t unused3;
> >    grub_uint8_t fork_offset;
> >    grub_uint8_t unused4[17];
> > -  union
> > -  {
> > -    char raw[156];
> > -    struct dir
> > -    {
> > -      struct grub_xfs_dir_header dirhead;
> > -      struct grub_xfs_dir_entry direntry[1];
> > -    } dir;
> > -    grub_xfs_extent extents[XFS_INODE_EXTENTS];
> > -    struct grub_xfs_btree_root btree;
> > -  } GRUB_PACKED data;
> 
> Can we make it grub_uint8_t xfs_v3_extra[76] or similar to avoid magic
> numbers later when computing data offset?

  Well, depending on fs version the size of "base" inode is different. I
have added defines like:
#define XFS_V2_INODE_SIZE sizeof(struct grub_xfs_inode)
#define XFS_V3_INODE_SIZE (XFS_V2_INODE_SIZE + 76)

That should make things clearer.

> >  } GRUB_PACKED;
> >  
> >  struct grub_xfs_dirblock_tail
> > @@ -157,6 +167,8 @@ struct grub_xfs_data
> >    int pos;
> >    int bsize;
> >    grub_uint32_t agsize;
> > +  unsigned int hasftype:1;
> > +  unsigned int hascrc:1;
> >    struct grub_fshelp_node diropen;
> >  };
> >  
> > @@ -164,6 +176,24 @@ static grub_dl_t my_mod;
> >  
> >  
> >  
> > +static int grub_xfs_sb_hascrc(struct grub_xfs_data *data)
> > +{
> > +  return (grub_be_to_cpu16(data->sblock.version) & XFS_SB_VERSION_NUMBITS) 
> > == 5;
> 
> Could you define name for magic constant?

  Done.

> Also to avoid run-time computation:
> data->sblock.version &
> grub_cpu_to_b16_compile_time(XFS_SB_VERSION_NUMBITS) ==
> grub_cpu_to_b16_compile_time(5)

  OK.

...
> > @@ -261,6 +279,96 @@ grub_xfs_inode_size(struct grub_xfs_data *data)
> >     return 1 << data->sblock.log2_inode;
> >  }
> >  
> > +static void *
> > +grub_xfs_inode_data(struct grub_xfs_inode *inode)
> > +{
> > +   if (inode->version <= 2)
> > +           return ((char *)inode) + 100;
> 
> That is sizeof(struct grub_xfs_inode) in your patch, right?
> 
> > +   return ((char *)inode) + 176;
> 
> Please avoid magic numbers. At least give it meaningful name, or as
> suggested above just define structure to contain it.

  Used defines above.

> > +}
> > +
> > +static struct grub_xfs_dir_entry *
> > +grub_xfs_inline_de(struct grub_xfs_dir_header *head)
> > +{
> > +   /*
> > +    * With small inode numbers the header is 4 bytes smaller because of
> > +    * smaller parent pointer
> > +    */
> > +   return (void *)(((char *)head) + sizeof(struct grub_xfs_dir_header) -
> > +           (head->largeino ? 0 : sizeof(grub_uint32_t)));
> > +}
> > +
> > +static grub_uint8_t *
> > +grub_xfs_inline_de_inopos(struct grub_xfs_data *data,
> > +                     struct grub_xfs_dir_entry *de)
> > +{
> > +   return ((grub_uint8_t *)(de + 1)) + de->len - 1 +
> The outer parens are somehow confusing.

  I can remove them but I find it better to explicitely show where the
typecast happens with parenthesis...

> 
> > +            (data->hasftype ? 1 : 0);
> > +}
> > +
> > +static struct grub_xfs_dir_entry *
> > +grub_xfs_inline_next_de(struct grub_xfs_data *data,
> > +                   struct grub_xfs_dir_header *head,
> > +                   struct grub_xfs_dir_entry *de)
> > +{
> > +  char *p = (char *)de + sizeof(struct grub_xfs_dir_entry) - 1 + de->len;
> > +
> > +  p += head->largeino ? sizeof(grub_uint64_t) : sizeof(grub_uint32_t);
> > +  if (data->hasftype)
> > +    p++;
> > +     
> > +  return (struct grub_xfs_dir_entry *)p;
> > +}
> > +
> > +static struct grub_xfs_dirblock_tail *
> > +grub_xfs_dir_tail(struct grub_xfs_data *data, void *dirblock)
> > +{
> > +  int dirblksize = 1 << (data->sblock.log2_bsize + 
> > data->sblock.log2_dirblk);
> > +
> > +  return (struct grub_xfs_dirblock_tail *)
> > +    ((char *)dirblock + dirblksize - sizeof (struct 
> > grub_xfs_dirblock_tail));
> > +}
> > +
> > +static struct grub_xfs_dir2_entry *
> > +grub_xfs_first_de(struct grub_xfs_data *data, void *dirblock)
> > +{
> > +  if (data->hascrc)
> > +    return (struct grub_xfs_dir2_entry *)((char *)dirblock + 64);
> > +  return (struct grub_xfs_dir2_entry *)((char *)dirblock + 16);
> > +}
> > +
> > +static inline int
> > +grub_xfs_round_dirent_size (int len)
> > +{
> > +  return (len + 7) & ~7;
> > +}
> 
> ALIGN_UP?
> 
> > +
> > +static struct grub_xfs_dir2_entry *
> > +grub_xfs_next_de(struct grub_xfs_data *data, struct grub_xfs_dir2_entry 
> > *de)
> > +{
> > +  int size = sizeof (struct grub_xfs_dir2_entry) + de->len + 2 /* Tag */;
> > +
> > +  if (data->hasftype)
> > +    size++;                /* File type */
> > +  return (struct grub_xfs_dir2_entry *)
> > +                   (((char *)de) + grub_xfs_round_dirent_size (size));
> 
> What's wrong with ALIGN_UP (size, 8)?

Nothing. I wasn't aware grub has the macro. Replaced.

> > +}
> > +
> > +static grub_uint64_t *
> > +grub_xfs_btree_keys(struct grub_xfs_data *data,
> > +               struct grub_xfs_btree_node *leaf)
> > +{
> > +  char *p = (char *)(leaf + 1);
> > +
> > +  if (data->hascrc)
> > +    p += 48;       /* crc, uuid, ... */
> > +  /*
> > +   * We have to first type to void * to avoid complaints about possible
> > +   * alignment issues on some architectures
> > +   */
> > +  return (grub_uint64_t *)(void *)p;
> 
> Leaving it as grub_uint64_t keys and using &keys[6] would avoid this
> warning as well, not? Also having keys[0] will likely simplify other
> places as well (we do require GCC in any case).

Well, the trouble with this is that we'd need two structures defined -
one for crc-enabled fs and one for old fs. That seemed like a wasted effort
to me when we could do:
  if (data->hascrc)
    p += 48;    /* crc, uuid, ... */
like the above. The same holds for inodes, directory entries, etc. I'd
prefer not to bloat the code with structure definitions we don't actually
use but if you really insisted, I could do that. So what do you think?

                                                                Honza
-- 
Jan Kara <address@hidden>
SUSE Labs, CR