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

[Andrei Borzenkov <arvidjaar@gmail.com>]

В Mon, 14 Jul 2014 17:21:31 +0200
Jan Kara <jack@suse.cz> пишет:

> Add support for new XFS on disk format. We have to handle optional
> filetype fields in directory entries, additional CRC, LSN, UUID entries
> in some structures, etc.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  grub-core/fs/xfs.c | 249 +++++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 177 insertions(+), 72 deletions(-)
> 
> diff --git a/grub-core/fs/xfs.c b/grub-core/fs/xfs.c
> index 7e247a32df5c..7f2582d33e02 100644
> --- a/grub-core/fs/xfs.c
> +++ b/grub-core/fs/xfs.c
> @@ -34,6 +34,15 @@ GRUB_MOD_LICENSE ("GPLv3+");
>  #define XFS_INODE_FORMAT_EXT	2
>  #define XFS_INODE_FORMAT_BTREE	3
>  
> +/* Superblock version field flags */
> +#define XFS_SB_VERSION_NUMBITS          0x000f
> +#define XFS_SB_VERSION_MOREBITSBIT      0x8000
> +
> +/* features2 field flags */
> +#define XFS_SB_VERSION2_FTYPE           0x00000200      /* inode type in dir */
> +
> +/* incompat feature flags */
> +#define XFS_SB_FEAT_INCOMPAT_FTYPE      (1 << 0)        /* filetype in dirent */
>  
>  struct grub_xfs_sblock
>  {
> @@ -45,7 +54,9 @@ struct grub_xfs_sblock
>    grub_uint64_t rootino;
>    grub_uint8_t unused3[20];
>    grub_uint32_t agsize;
> -  grub_uint8_t unused4[20];
> +  grub_uint8_t unused4[12];
> +  grub_uint16_t version;
> +  grub_uint8_t unused5[6];
>    grub_uint8_t label[12];
>    grub_uint8_t log2_bsize;
>    grub_uint8_t log2_sect;
> @@ -54,12 +65,19 @@ struct grub_xfs_sblock
>    grub_uint8_t log2_agblk;
>    grub_uint8_t unused6[67];
>    grub_uint8_t log2_dirblk;
> +  grub_uint8_t unused7[7];
> +  grub_uint32_t features2;
> +  grub_uint8_t unused8[4];
> +  grub_uint32_t sb_features_compat;
> +  grub_uint32_t sb_features_ro_compat;
> +  grub_uint32_t sb_features_incompat;
> +  grub_uint32_t sb_features_log_incompat;
>  } GRUB_PACKED;
>  
>  struct grub_xfs_dir_header
>  {
>    grub_uint8_t count;
> -  grub_uint8_t smallino;
> +  grub_uint8_t largeino;
>    union
>    {
>      grub_uint32_t i4;
> @@ -67,14 +85,16 @@ struct grub_xfs_dir_header
>    } GRUB_PACKED parent;
>  } GRUB_PACKED;
>  
> +/* Structure for directory entry inlined in the inode */
>  struct grub_xfs_dir_entry
>  {
>    grub_uint8_t len;
>    grub_uint16_t offset;
>    char name[1];
> -  /* Inode number follows, 32 bits.  */
> +  /* Inode number follows, 32 / 64 bits.  */
>  } GRUB_PACKED;
>  
> +/* Structure for directory entry in a block */
>  struct grub_xfs_dir2_entry
>  {
>    grub_uint64_t inode;
> @@ -90,7 +110,8 @@ struct grub_xfs_btree_node
>    grub_uint16_t numrecs;
>    grub_uint64_t left;
>    grub_uint64_t right;
> -  grub_uint64_t keys[1];
> +  /* In V5 here follow crc, uuid, etc. */
> +  /* Then follow keys and block pointers */
>  }  GRUB_PACKED;
>  
>  struct grub_xfs_btree_root
> @@ -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?

>  } 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;
>  
>  \f
>  
> +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?

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)

> +}
> +
> +static int grub_xfs_sb_hasftype(struct grub_xfs_data *data)
> +{
> +  grub_uint32_t version = grub_be_to_cpu16(data->sblock.version);
> +
> +  if ((version & XFS_SB_VERSION_NUMBITS) == 5 &&
> +      grub_be_to_cpu32(data->sblock.sb_features_incompat) & XFS_SB_FEAT_INCOMPAT_FTYPE)
> +    return 1;
> +  if (version & XFS_SB_VERSION_MOREBITSBIT &&
> +      grub_be_to_cpu32(data->sblock.features2) & XFS_SB_VERSION2_FTYPE)
> +    return 1;
> +  return 0;
> +}
> +

Same. All are constants, so *_compile_time.

>  /* Filetype information as used in inodes.  */
>  #define FILETYPE_INO_MASK	0170000
>  #define FILETYPE_INO_REG	0100000
> @@ -219,18 +249,6 @@ GRUB_XFS_EXTENT_SIZE (grub_xfs_extent *exts, int ex)
>    return (grub_be_to_cpu32 (exts[ex][3]) & ((1 << 21) - 1));
>  }
>  
> -static inline int
> -GRUB_XFS_ROUND_TO_DIRENT (int pos)
> -{
> -  return ((((pos) + 8 - 1) / 8) * 8);
> -}
> -
> -static inline int
> -GRUB_XFS_NEXT_DIRENT (int pos, int len)
> -{
> -  return (pos) + GRUB_XFS_ROUND_TO_DIRENT (8 + 1 + len + 2);
> -}
> -
>  \f
>  static inline grub_uint64_t
>  grub_xfs_inode_block (struct grub_xfs_data *data,
> @@ -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.

> +}
> +
> +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.

> +		 (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)?

> +}
> +
> +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).

> +}
> +
>  static grub_err_t
>  grub_xfs_read_inode (struct grub_xfs_data *data, grub_uint64_t ino,
>  		     struct grub_xfs_inode *inode)
> @@ -268,6 +376,9 @@ grub_xfs_read_inode (struct grub_xfs_data *data, grub_uint64_t ino,
>    grub_uint64_t block = grub_xfs_inode_block (data, ino);
>    int offset = grub_xfs_inode_offset (data, ino);
>  
> +  grub_dprintf("xfs", "Reading inode (%llu) - %llu, %d\n",
> +	       (unsigned long long) ino,
> +	       (unsigned long long) block, offset);
>    /* Read the inode.  */
>    if (grub_disk_read (data->disk, block, offset, grub_xfs_inode_size(data),
>  		      inode))
> @@ -290,6 +401,7 @@ grub_xfs_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
>  
>    if (node->inode.format == XFS_INODE_FORMAT_BTREE)
>      {
> +      struct grub_xfs_btree_root *root;
>        const grub_uint64_t *keys;
>        int recoffset;
>  
> @@ -297,15 +409,15 @@ grub_xfs_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
>        if (leaf == 0)
>          return 0;
>  
> -      nrec = grub_be_to_cpu16 (node->inode.data.btree.numrecs);
> -      keys = &node->inode.data.btree.keys[0];
> +      root = grub_xfs_inode_data(&node->inode);
> +      nrec = grub_be_to_cpu16 (root->numrecs);
> +      keys = &root->keys[0];
>        if (node->inode.fork_offset)
>  	recoffset = (node->inode.fork_offset - 1) / 2;
>        else
>  	recoffset = (grub_xfs_inode_size(node->data)
> -		     - ((char *) &node->inode.data.btree.keys
> -			- (char *) &node->inode))
> -	  / (2 * sizeof (grub_uint64_t));
> +		     - ((char *) keys - (char *) &node->inode))
> +				/ (2 * sizeof (grub_uint64_t));
>        do
>          {
>            int i;
> @@ -327,7 +439,10 @@ grub_xfs_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
>                                0, node->data->bsize, leaf))
>              return 0;
>  
> -          if (grub_strncmp ((char *) leaf->magic, "BMAP", 4))
> +	  if ((!node->data->hascrc &&
> +	       grub_strncmp ((char *) leaf->magic, "BMAP", 4)) ||
> +	      (node->data->hascrc &&
> +	       grub_strncmp ((char *) leaf->magic, "BMA3", 4)))
>              {
>                grub_free (leaf);
>                grub_error (GRUB_ERR_BAD_FS, "not a correct XFS BMAP node");
> @@ -335,8 +450,8 @@ grub_xfs_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
>              }
>  
>            nrec = grub_be_to_cpu16 (leaf->numrecs);
> -          keys = &leaf->keys[0];
> -	  recoffset = ((node->data->bsize - ((char *) &leaf->keys
> +          keys = grub_xfs_btree_keys(node->data, leaf);
> +	  recoffset = ((node->data->bsize - ((char *) keys
>  					     - (char *) leaf))
>  		       / (2 * sizeof (grub_uint64_t)));
>  	}
> @@ -346,7 +461,7 @@ grub_xfs_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
>    else if (node->inode.format == XFS_INODE_FORMAT_EXT)
>      {
>        nrec = grub_be_to_cpu32 (node->inode.nextents);
> -      exts = &node->inode.data.extents[0];
> +      exts = grub_xfs_inode_data(&node->inode);
>      }
>    else
>      {
> @@ -404,7 +519,7 @@ grub_xfs_read_symlink (grub_fshelp_node_t node)
>    switch (node->inode.format)
>      {
>      case XFS_INODE_FORMAT_INO:
> -      return grub_strndup (node->inode.data.raw, size);
> +      return grub_strndup (grub_xfs_inode_data(&node->inode), size);
>  
>      case XFS_INODE_FORMAT_EXT:
>        {
> @@ -501,23 +616,18 @@ grub_xfs_iterate_dir (grub_fshelp_node_t dir,
>      {
>      case XFS_INODE_FORMAT_INO:
>        {
> -	struct grub_xfs_dir_entry *de = &diro->inode.data.dir.direntry[0];
> -	int smallino = !diro->inode.data.dir.dirhead.smallino;
> +	struct grub_xfs_dir_header *head = grub_xfs_inode_data(&diro->inode);
> +	struct grub_xfs_dir_entry *de = grub_xfs_inline_de(head);
> +	int smallino = !head->largeino;
>  	int i;
>  	grub_uint64_t parent;
>  
>  	/* If small inode numbers are used to pack the direntry, the
>  	   parent inode number is small too.  */
>  	if (smallino)
> -	  {
> -	    parent = grub_be_to_cpu32 (diro->inode.data.dir.dirhead.parent.i4);
> -	    /* The header is a bit smaller than usual.  */
> -	    de = (struct grub_xfs_dir_entry *) ((char *) de - 4);
> -	  }
> +	  parent = grub_be_to_cpu32 (head->parent.i4);
>  	else
> -	  {
> -	    parent = grub_be_to_cpu64(diro->inode.data.dir.dirhead.parent.i8);
> -	  }
> +	  parent = grub_be_to_cpu64 (head->parent.i8);
>  
>  	/* Synthesize the direntries for `.' and `..'.  */
>  	if (iterate_dir_call_hook (diro->ino, ".", &ctx))
> @@ -526,12 +636,10 @@ grub_xfs_iterate_dir (grub_fshelp_node_t dir,
>  	if (iterate_dir_call_hook (parent, "..", &ctx))
>  	  return 1;
>  
> -	for (i = 0; i < diro->inode.data.dir.dirhead.count; i++)
> +	for (i = 0; i < head->count; i++)
>  	  {
>  	    grub_uint64_t ino;
> -	    grub_uint8_t *inopos = (((grub_uint8_t *) de)
> -			    + sizeof (struct grub_xfs_dir_entry)
> -			    + de->len - 1);
> +	    grub_uint8_t *inopos = grub_xfs_inline_de_inopos(dir->data, de);
>  	    grub_uint8_t c;
>  
>  	    /* inopos might be unaligned.  */
> @@ -556,10 +664,7 @@ grub_xfs_iterate_dir (grub_fshelp_node_t dir,
>  	      return 1;
>  	    de->name[de->len] = c;
>  
> -	    de = ((struct grub_xfs_dir_entry *)
> -		  (((char *) de)+ sizeof (struct grub_xfs_dir_entry) + de->len
> -		   + ((smallino ? sizeof (grub_uint32_t)
> -		       : sizeof (grub_uint64_t))) - 1));
> +	    de = grub_xfs_inline_next_de(dir->data, head, de);
>  	  }
>  	break;
>        }
> @@ -586,15 +691,11 @@ grub_xfs_iterate_dir (grub_fshelp_node_t dir,
>  		    >> dirblk_log2);
>  	     blk++)
>  	  {
> -	    /* The header is skipped, the first direntry is stored
> -	       from byte 16.  */
> -	    int pos = 16;
> +	    struct grub_xfs_dir2_entry *direntry =
> +					grub_xfs_first_de(dir->data, dirblock);
>  	    int entries;
> -	    int tail_start = (dirblk_size
> -			      - sizeof (struct grub_xfs_dirblock_tail));
> -
> -	    struct grub_xfs_dirblock_tail *tail;
> -	    tail = (struct grub_xfs_dirblock_tail *) &dirblock[tail_start];
> +	    struct grub_xfs_dirblock_tail *tail =
> +					grub_xfs_dir_tail(dir->data, dirblock);
>  
>  	    numread = grub_xfs_read_file (dir, 0, 0,
>  					  blk << dirblk_log2,
> @@ -606,13 +707,11 @@ grub_xfs_iterate_dir (grub_fshelp_node_t dir,
>  		       - grub_be_to_cpu32 (tail->leaf_stale));
>  
>  	    /* Iterate over all entries within this block.  */
> -	    while (pos < tail_start)
> +	    while ((char *)direntry < (char *)tail)
>  	      {
> -		struct grub_xfs_dir2_entry *direntry;
>  		grub_uint8_t *freetag;
>  		char *filename;
>  
> -		direntry = (struct grub_xfs_dir2_entry *) &dirblock[pos];
>  		freetag = (grub_uint8_t *) direntry;
>  
>  		if (grub_get_unaligned16 (freetag) == 0XFFFF)
> @@ -620,14 +719,16 @@ grub_xfs_iterate_dir (grub_fshelp_node_t dir,
>  		    grub_uint8_t *skip = (freetag + sizeof (grub_uint16_t));
>  
>  		    /* This entry is not used, go to the next one.  */
> -		    pos += grub_be_to_cpu16 (grub_get_unaligned16 (skip));
> +		    direntry = (struct grub_xfs_dir2_entry *)
> +				(((char *)direntry) +
> +				grub_be_to_cpu16 (grub_get_unaligned16 (skip)));
>  
>  		    continue;
>  		  }
>  
> -		filename = &dirblock[pos + sizeof (*direntry)];
> -		/* The byte after the filename is for the tag, which
> -		   is not used by GRUB.  So it can be overwritten.  */
> +		filename = (char *)(direntry + 1);
> +		/* The byte after the filename is for the filetype, padding, or
> +		   tag, which is not used by GRUB.  So it can be overwritten. */
>  		filename[direntry->len] = '\0';
>  
>  		if (iterate_dir_call_hook (grub_be_to_cpu64(direntry->inode), 
> @@ -644,8 +745,7 @@ grub_xfs_iterate_dir (grub_fshelp_node_t dir,
>  		  break;
>  
>  		/* Select the next directory entry.  */
> -		pos = GRUB_XFS_NEXT_DIRENT (pos, direntry->len);
> -		pos = GRUB_XFS_ROUND_TO_DIRENT (pos);
> +		direntry = grub_xfs_next_de(dir->data, direntry);
>  	      }
>  	  }
>  	grub_free (dirblock);
> @@ -670,6 +770,7 @@ grub_xfs_mount (grub_disk_t disk)
>    if (!data)
>      return 0;
>  
> +  grub_dprintf("xfs", "Reading sb\n");
>    /* Read the superblock.  */
>    if (grub_disk_read (disk, 0, 0,
>  		      sizeof (struct grub_xfs_sblock), &data->sblock))
> @@ -697,9 +798,13 @@ grub_xfs_mount (grub_disk_t disk)
>    data->diropen.inode_read = 1;
>    data->bsize = grub_be_to_cpu32 (data->sblock.bsize);
>    data->agsize = grub_be_to_cpu32 (data->sblock.agsize);
> +  data->hasftype = grub_xfs_sb_hasftype(data);
> +  data->hascrc = grub_xfs_sb_hascrc(data);
>  
>    data->disk = disk;
>    data->pos = 0;
> +  grub_dprintf("xfs", "Reading root ino %llu\n",
> +	       (unsigned long long) grub_cpu_to_be64(data->sblock.rootino));
>  
>    grub_xfs_read_inode (data, data->diropen.ino, &data->diropen.inode);
>