Re: [Patch] Robustly search for ZFS labels & uberblocks

["Vladimir 'φ-coder/phcoder' Serbinenko" <phcoder@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 36080 bytes --]

On 19.09.2011 20:45, Zachary Bedell wrote:
> Greetings,
>
> In working with ZFSonLinux, I found myself with a number of somewhat inconstant pools which failed either when running grub-probe or at boot time.  In all cases, these pools were importable by the ZFS driver and completed a `zpool scrub` with no errors reported, but still Grub wasn't able to touch the pools.  In most (but not all) cases, running `zpool scrub` made the pool accessible to Grub again, though obviously that was less than helpful for boot-time failures.
>
Patch is pretty difficult to read due to spacing changes. Please use -bB
or indent
> I'm including an (unfortunately rather large) patch to Grub's ZFS code which fixes several edge cases where Grub is unable to read a pool which is otherwise valid as far as the full ZFS driver is concerned.  Changes include:
>
> ashift:
>
> * Support non-default values of ashift pool attribute.  When ashift is increased beyond 10, the size of the uberblocks changes.  Scan to find uberblocks must account for this and skip over the extra padding.  Based on patch to Grub 0.97 by Hans Rosenfeld at http://www.illumos.org/issues/1303
>
>
> Label parsing changes:
>
> * Access the two end-device labels at label-size aligned location rather than device_end-(2*label_size).  On-disk spec document incorrectly describes end-label locations.  Adapted from Grub 0.97 behavior.
>
> * Scan all readable labels for uberblocks and accept the one with the highest txg/create date.  Previously UB scan would stop if a valid UB was found in Label 0 without checking for newer UB's in the other labels.  All labels must be scanned as a more recent block may be found in the other labels in certain circumstances.  This fixes a case where Grub would be unable to access a ZFS system unless the pool was scrubbed first (but ZFS itself saw no problems & scrub reported zero errors).
>
> * Verify that uberblock txg is greater than that of the label before accepting the UB so that partially written UB's are not considered.
>
> * Verify that underlying data pointed by uberblock ub_rootbp has valid checksums before accepting the UB.  This gives some possibility of booting from a technically invalid pool and effectively falls back to older uberblocks in a case where the correct uberblock is damaged.
>
> * Store pool uuid found during zfs_mount to grub_zfs_data in order to prevent duplicated logic in zfs_uuid.
>
>
> Data integrity:
>
> * Verify checksum in grub_read_data before accepting block.  Attempt to read backup DVA's if checksum on first fails.  Previously backup DVA's were checked only if the physical read failed, not if bad data was read without error.  This resulted in pools which were valid and readable by ZFS (and transparently healed by ZFS' read behavior) being rejected by Grub.
>
>
> Logging/Debugging:
>
> * Provide more debugging output describing inconsistencies found in pool.
>
> * Remove superflous debugging output to reduce bootup time in verbose mode to a mangeable level (~30min down to ~5min to boot w/ 'debug=all' in grub.cfg)
>
>
>
> I do apologies for the monolithic patch.  I took a second look at the changes to try to pull certain elements apart, but I ended up in several situations where the test pools I have snapshotted in VM's each hit two or more of the issues above making testing in isolation difficult.  The total change set is able to probe and boot from all of the odd (as well as all of the normal) pools that I have on hand.  
>
> If remixing this patch in some way would help in integrating it, I'd welcome any feedback on how to better present the changes.
>
> Best regards,
> Zac Bedell
>
>
> grub-zfs-ashift-label.patch
>
>
> diff --git a/grub-core/fs/zfs/zfs.c b/grub-core/fs/zfs/zfs.c
> index 1eea13b..d03e19b 100644
> --- a/grub-core/fs/zfs/zfs.c
> +++ b/grub-core/fs/zfs/zfs.c
> @@ -75,9 +75,21 @@ GRUB_MOD_LICENSE ("GPLv3+");
>  static grub_dl_t my_mod;
>  #endif
>  
> +/*
> + * Macros to get fields in a bp or DVA.
> + */
>  #define	P2PHASE(x, align)		((x) & ((align) - 1))
>  #define	DVA_OFFSET_TO_PHYS_SECTOR(offset) \
>  	((offset + VDEV_LABEL_START_SIZE) >> SPA_MINBLOCKSHIFT)
> +	
> +/*
> + * return x rounded down to an align boundary
> + * eg, P2ALIGN(1200, 1024) == 1024 (1*align)
> + * eg, P2ALIGN(1024, 1024) == 1024 (1*align)
> + * eg, P2ALIGN(0x1234, 0x100) == 0x1200 (0x12*align)
> + * eg, P2ALIGN(0x5600, 0x100) == 0x5600 (0x56*align)
> + */
> +#define	P2ALIGN(x, align)		((x) & -(align))
>  
We already have such macros, they're called ALIGN_UP and ALIGN_DOWN.
>  /*
>   * FAT ZAP data structures
> @@ -147,6 +159,14 @@ struct grub_zfs_data
>    grub_uint64_t file_start;
>    grub_uint64_t file_end;
>  
> +  /* XXX: ashift is per vdev, not per pool.  We currently only ever touch
> +   * a single vdev, but when/if raid-z or stripes are supported, this
> +   * may need revision.
> +   */
> +  grub_uint64_t vdev_ashift;
> +  grub_uint64_t label_txg;
> +  grub_uint64_t pool_guid;
> +
We have multidevice support and we have a special structure for it.
Moreover it seems that you need it only on initial scan and so no need
to permanently store it.
>    /* cache for a dnode block */
>    dnode_phys_t *dnode_buf;
>    dnode_phys_t *dnode_mdn;
> @@ -192,7 +212,11 @@ static decomp_entry_t decomp_table[ZIO_COMPRESS_FUNCTIONS] = {
>  
>  static grub_err_t zio_read_data (blkptr_t * bp, grub_zfs_endian_t endian,
>  				 void *buf, struct grub_zfs_data *data);
> -
> +				
> +static grub_err_t
> +zio_read (blkptr_t * bp, grub_zfs_endian_t endian, void **buf, 
> +	  grub_size_t *size, struct grub_zfs_data *data);
> +	
>  /*
>   * Our own version of log2().  Same thing as highbit()-1.
>   */
> @@ -271,6 +295,7 @@ zio_checksum_verify (zio_cksum_t zc, grub_uint32_t checksum,
>        || (actual_cksum.zc_word[2] != zc.zc_word[2]) 
>        || (actual_cksum.zc_word[3] != zc.zc_word[3]))
>      {
> +      /*
>        grub_dprintf ("zfs", "checksum %d verification failed\n", checksum);
>        grub_dprintf ("zfs", "actual checksum %16llx %16llx %16llx %16llx\n",
>  		    (unsigned long long) actual_cksum.zc_word[0], 
> @@ -282,6 +307,7 @@ zio_checksum_verify (zio_cksum_t zc, grub_uint32_t checksum,
>  		    (unsigned long long) zc.zc_word[1],
>  		    (unsigned long long) zc.zc_word[2], 
>  		    (unsigned long long) zc.zc_word[3]);
> +	  */
Please don't comment out the code. This particular one is very useful
actually.
>        return grub_error (GRUB_ERR_BAD_FS, "checksum verification failed");
>      }
>  
> @@ -332,17 +358,22 @@ vdev_uberblock_compare (uberblock_t * ub1, uberblock_t * ub2)
>   * Three pieces of information are needed to verify an uberblock: the magic
>   * number, the version number, and the checksum.
>   *
> - * Currently Implemented: version number, magic number
> + * Currently Implemented: version number, magic number, label txg
>   * Need to Implement: checksum
>   *
>   */
>  static grub_err_t
> -uberblock_verify (uberblock_phys_t * ub, int offset)
> +uberblock_verify (uberblock_t * uber, int offset, struct grub_zfs_data *data)
>  {
> -  uberblock_t *uber = &ub->ubp_uberblock;
>    grub_err_t err;
>    grub_zfs_endian_t endian = UNKNOWN_ENDIAN;
>    zio_cksum_t zc;
> +  void *osp = NULL;
> +  grub_size_t ospsize;
> +
> +  if (uber->ub_txg < data->label_txg)
> +    return grub_error (GRUB_ERR_BAD_FS, 
> +	"ignoring partially written label: uber_txg < label_txg");
>  
>    if (grub_zfs_to_cpu64 (uber->ub_magic, LITTLE_ENDIAN) == UBERBLOCK_MAGIC
>        && grub_zfs_to_cpu64 (uber->ub_version, LITTLE_ENDIAN) > 0 
> @@ -358,10 +389,21 @@ uberblock_verify (uberblock_phys_t * ub, int offset)
>      return grub_error (GRUB_ERR_BAD_FS, "invalid uberblock magic");
>  
>    grub_memset (&zc, 0, sizeof (zc));
> -
>    zc.zc_word[0] = grub_cpu_to_zfs64 (offset, endian);
>    err = zio_checksum_verify (zc, ZIO_CHECKSUM_LABEL, endian,
> -			     (char *) ub, UBERBLOCK_SIZE);
> +			     (char *) uber, UBERBLOCK_SIZE(data->vdev_ashift));
> +
> +  if (!err)
> +    {
> +      // Check that the data pointed by the rootbp is usable.
> +      void *osp = NULL;
> +      grub_size_t ospsize;
> +      err = zio_read (&uber->ub_rootbp, endian, &osp, &ospsize, data);
> +      grub_free (osp);
> +
> +      if (!err && ospsize < OBJSET_PHYS_SIZE_V14)
> +		return grub_error (GRUB_ERR_BAD_FS, "uberblock rootbp points to invalid data");
> +    }
>  
This should be if (err) return err; ....
>    return err;
>  }
> @@ -372,32 +414,40 @@ uberblock_verify (uberblock_phys_t * ub, int offset)
>   *    Success - Pointer to the best uberblock.
>   *    Failure - NULL
>   */
> -static uberblock_phys_t *
> -find_bestub (uberblock_phys_t * ub_array, grub_disk_addr_t sector)
> +static uberblock_t *
> +find_bestub (char *ub_array, struct grub_zfs_data *data)
>  {
> -  uberblock_phys_t *ubbest = NULL;
> -  int i;
> -  grub_disk_addr_t offset;
> +  const grub_disk_addr_t sector = data->vdev_phys_sector;
> +  uberblock_t *ubbest = NULL;
> +  uberblock_t *ubnext;
> +  unsigned int i, offset, pickedub = 0;
>    grub_err_t err = GRUB_ERR_NONE;
>  
> -  for (i = 0; i < (VDEV_UBERBLOCK_RING >> VDEV_UBERBLOCK_SHIFT); i++)
> +  const unsigned int UBCOUNT = UBERBLOCK_COUNT (data->vdev_ashift);
> +  const grub_uint64_t UBBYTES = UBERBLOCK_SIZE (data->vdev_ashift);
> +
> +  for (i = 0; i < UBCOUNT; i++)
>      {
> -      offset = (sector << SPA_MINBLOCKSHIFT) + VDEV_PHYS_SIZE
> -	+ (i << VDEV_UBERBLOCK_SHIFT);
> +      ubnext = (uberblock_t *) (i * UBBYTES + ub_array);
> +      offset = (sector << SPA_MINBLOCKSHIFT) + VDEV_PHYS_SIZE + (i * UBBYTES);
>  
> -      err = uberblock_verify (&ub_array[i], offset);
> +      err = uberblock_verify (ubnext, offset, data);
>        if (err)
> -	{
> -	  grub_errno = GRUB_ERR_NONE;
> -	  continue;
> -	}
> -      if (ubbest == NULL 
> -	  || vdev_uberblock_compare (&(ub_array[i].ubp_uberblock),
> -				     &(ubbest->ubp_uberblock)) > 0)
> -	ubbest = &ub_array[i];
> +		{
> +	      grub_errno = GRUB_ERR_NONE;
> +	      continue;
> +		}
> +      if (ubbest == NULL || vdev_uberblock_compare (ubnext, ubbest) > 0)
> +		{
> +	  	  ubbest = ubnext;
> +	      pickedub = i;
> +		}
>      }
>    if (!ubbest)
>      grub_errno = err;
> +  else
> +    grub_dprintf ("zfs", "Found best uberblock at idx %d, txg %llu\n",
> +		  pickedub, (unsigned long long) ubbest->ub_txg);
>  
Again the same error. Don't put such if's.
>    return (ubbest);
>  }
> @@ -412,9 +462,6 @@ get_psize (blkptr_t * bp, grub_zfs_endian_t endian)
>  static grub_uint64_t
>  dva_get_offset (dva_t * dva, grub_zfs_endian_t endian)
>  {
> -  grub_dprintf ("zfs", "dva=%llx, %llx\n", 
> -		(unsigned long long) dva->dva_word[0], 
> -		(unsigned long long) dva->dva_word[1]);
>    return grub_zfs_to_cpu64 ((dva)->dva_word[1], 
>  			    endian) << SPA_MINBLOCKSHIFT;
>  }
Please don't mix dprintf removals with actual changes. Just makes more
difficult to read.
> @@ -440,11 +487,9 @@ zio_read_gang (blkptr_t * bp, grub_zfs_endian_t endian, dva_t * dva, void *buf,
>    zio_gb = grub_malloc (SPA_GANGBLOCKSIZE);
>    if (!zio_gb)
>      return grub_errno;
> -  grub_dprintf ("zfs", endian == LITTLE_ENDIAN ? "little-endian gang\n"
> -		:"big-endian gang\n");
> +
>    offset = dva_get_offset (dva, endian);
>    sector = DVA_OFFSET_TO_PHYS_SECTOR (offset);
> -  grub_dprintf ("zfs", "offset=%llx\n", (unsigned long long) offset);
>  
>    /* read in the gang block header */
>    err = grub_disk_read (data->disk, sector, 0, SPA_GANGBLOCKSIZE,
> @@ -515,10 +560,20 @@ zio_read_data (blkptr_t * bp, grub_zfs_endian_t endian, void *buf,
>  	  sector = DVA_OFFSET_TO_PHYS_SECTOR (offset);
>  	  err = grub_disk_read (data->disk, sector, 0, psize, buf); 
>  	}
> -      if (!err)
> -	return GRUB_ERR_NONE;
> -      grub_errno = GRUB_ERR_NONE;
> -    }
> +
> +    if (!err)
> +  	  {
> +	  	// Check the underlying checksum before we rule this DVA as "good"
> +	  	grub_uint32_t checkalgo = (grub_zfs_to_cpu64 ((bp)->blk_prop, endian) >> 40) & 0xff;
> +
> +	  	err = zio_checksum_verify (bp->blk_cksum, checkalgo, endian, buf, psize);
> +	  	if (!err)
> +          return GRUB_ERR_NONE;
> +	  }
> +	
> +       // If read failed or checksum bad, reset the error.  Hopefully we've got some more DVA's to try.
You need to correctly handle the case when you don't. See e.g. the
mirror reading code. Also we don't use C++ style comments. I fail to see
the structure and need of this checksum move.
> +       grub_errno = GRUB_ERR_NONE;
> +     }
>  
>    if (!err)
>      err = grub_error (GRUB_ERR_BAD_FS, "couldn't find a valid DVA");
> @@ -539,12 +594,9 @@ zio_read (blkptr_t * bp, grub_zfs_endian_t endian, void **buf,
>    unsigned int comp;
>    char *compbuf = NULL;
>    grub_err_t err;
> -  zio_cksum_t zc = bp->blk_cksum;
> -  grub_uint32_t checksum;
>  
>    *buf = NULL;
>  
> -  checksum = (grub_zfs_to_cpu64((bp)->blk_prop, endian) >> 40) & 0xff;
>    comp = (grub_zfs_to_cpu64((bp)->blk_prop, endian)>>32) & 0xff;
>    lsize = (BP_IS_HOLE(bp) ? 0 :
>  	   (((grub_zfs_to_cpu64 ((bp)->blk_prop, endian) & 0xffff) + 1)
> @@ -571,7 +623,6 @@ zio_read (blkptr_t * bp, grub_zfs_endian_t endian, void **buf,
>    else
>      compbuf = *buf = grub_malloc (lsize);
>  
> -  grub_dprintf ("zfs", "endian = %d\n", endian);
>    err = zio_read_data (bp, endian, compbuf, data);
>    if (err)
>      {
> @@ -580,15 +631,6 @@ zio_read (blkptr_t * bp, grub_zfs_endian_t endian, void **buf,
>        return err;
>      }
>  
> -  err = zio_checksum_verify (zc, checksum, endian, compbuf, psize);
> -  if (err)
> -    {
> -      grub_dprintf ("zfs", "incorrect checksum\n");
> -      grub_free (compbuf);
> -      *buf = NULL;
> -      return err;
> -    }
> -
>    if (comp != ZIO_COMPRESS_OFF)
>      {
>        *buf = grub_malloc (lsize);
> @@ -635,7 +677,6 @@ dmu_read (dnode_end_t * dn, grub_uint64_t blkid, void **buf,
>    endian = dn->endian;
>    for (level = dn->dn.dn_nlevels - 1; level >= 0; level--)
>      {
> -      grub_dprintf ("zfs", "endian = %d\n", endian);
>        idx = (blkid >> (epbs * level)) & ((1 << epbs) - 1);
>        *bp = bp_array[idx];
>        if (bp_array != dn->dn.dn_blkptr)
> @@ -661,12 +702,10 @@ dmu_read (dnode_end_t * dn, grub_uint64_t blkid, void **buf,
>  	}
>        if (level == 0)
>  	{
> -	  grub_dprintf ("zfs", "endian = %d\n", endian);
>  	  err = zio_read (bp, endian, buf, 0, data);
>  	  endian = (grub_zfs_to_cpu64 (bp->blk_prop, endian) >> 63) & 1;
>  	  break;
>  	}
> -      grub_dprintf ("zfs", "endian = %d\n", endian);
>        err = zio_read (bp, endian, &tmpbuf, 0, data);
>        endian = (grub_zfs_to_cpu64 (bp->blk_prop, endian) >> 63) & 1;
>        if (err)
> @@ -717,9 +756,6 @@ mzap_iterate (mzap_phys_t * zapobj, grub_zfs_endian_t endian, int objsize,
>    chunks = objsize / MZAP_ENT_LEN - 1;
>    for (i = 0; i < chunks; i++)
>      {
> -      grub_dprintf ("zfs", "zap: name = %s, value = %llx, cd = %x\n",
> -		    mzap_ent[i].mze_name, (long long)mzap_ent[i].mze_value,
> -		    (int)mzap_ent[i].mze_cd);
>        if (hook (mzap_ent[i].mze_name, 
>  		grub_zfs_to_cpu64 (mzap_ent[i].mze_value, endian)))
>  	return 1;
> @@ -849,8 +885,6 @@ zap_leaf_lookup (zap_leaf_phys_t * l, grub_zfs_endian_t endian,
>        if (grub_zfs_to_cpu64 (le->le_hash,endian) != h)
>  	continue;
>  
> -      grub_dprintf ("zfs", "fzap: length %d\n", (int) le->le_name_length);
> -
>        if (zap_leaf_array_equal (l, endian, blksft, 
>  				grub_zfs_to_cpu16 (le->le_name_chunk,endian),
>  				grub_zfs_to_cpu16 (le->le_name_length, endian),
> @@ -1050,22 +1084,16 @@ zap_lookup (dnode_end_t * zap_dnode, char *name, grub_uint64_t * val,
>      return err;
>    block_type = grub_zfs_to_cpu64 (*((grub_uint64_t *) zapbuf), endian);
>  
> -  grub_dprintf ("zfs", "zap read\n");
> -
>    if (block_type == ZBT_MICRO)
>      {
> -      grub_dprintf ("zfs", "micro zap\n");
>        err = (mzap_lookup (zapbuf, endian, size, name, val));
> -      grub_dprintf ("zfs", "returned %d\n", err);      
>        grub_free (zapbuf);
>        return err;
>      }
>    else if (block_type == ZBT_HEADER)
>      {
> -      grub_dprintf ("zfs", "fat zap\n");
>        /* this is a fat zap */
>        err = (fzap_lookup (zap_dnode, zapbuf, name, val, data));
> -      grub_dprintf ("zfs", "returned %d\n", err);      
>        grub_free (zapbuf);
>        return err;
>      }
> @@ -1092,18 +1120,14 @@ zap_iterate (dnode_end_t * zap_dnode,
>      return 0;
>    block_type = grub_zfs_to_cpu64 (*((grub_uint64_t *) zapbuf), endian);
>  
> -  grub_dprintf ("zfs", "zap read\n");
> -
>    if (block_type == ZBT_MICRO)
>      {
> -      grub_dprintf ("zfs", "micro zap\n");
>        ret = mzap_iterate (zapbuf, endian, size, hook);
>        grub_free (zapbuf);
>        return ret;
>      }
>    else if (block_type == ZBT_HEADER)
>      {
> -      grub_dprintf ("zfs", "fat zap\n");
>        /* this is a fat zap */
>        ret = fzap_iterate (zap_dnode, zapbuf, hook, data);
>        grub_free (zapbuf);
> @@ -1150,12 +1174,9 @@ dnode_get (dnode_end_t * mdn, grub_uint64_t objnum, grub_uint8_t type,
>        return GRUB_ERR_NONE;
>      }
>  
> -  grub_dprintf ("zfs", "endian = %d, blkid=%llx\n", mdn->endian, 
> -		(unsigned long long) blkid);
>    err = dmu_read (mdn, blkid, &dnbuf, &endian, data);
>    if (err)
>      return err;
> -  grub_dprintf ("zfs", "alive\n");
>  
>    grub_free (data->dnode_buf);
>    grub_free (data->dnode_mdn);
> @@ -1426,27 +1447,19 @@ get_filesystem_dnode (dnode_end_t * mosmdn, char *fsname,
>    grub_uint64_t objnum;
>    grub_err_t err;
>  
> -  grub_dprintf ("zfs", "endian = %d\n", mosmdn->endian);
> -
>    err = dnode_get (mosmdn, DMU_POOL_DIRECTORY_OBJECT, 
>  		   DMU_OT_OBJECT_DIRECTORY, mdn, data);
>    if (err)
>      return err;
>  
> -  grub_dprintf ("zfs", "alive\n");
> -
>    err = zap_lookup (mdn, DMU_POOL_ROOT_DATASET, &objnum, data);
>    if (err)
>      return err;
>  
> -  grub_dprintf ("zfs", "alive\n");
> -
>    err = dnode_get (mosmdn, objnum, DMU_OT_DSL_DIR, mdn, data);
>    if (err)
>      return err;
>  
> -  grub_dprintf ("zfs", "alive\n");
> -
>    while (*fsname)
>      {
>        grub_uint64_t childobj;
> @@ -1491,8 +1504,6 @@ make_mdn (dnode_end_t * mdn, struct grub_zfs_data *data)
>    grub_size_t ospsize;
>    grub_err_t err;
>  
> -  grub_dprintf ("zfs", "endian = %d\n", mdn->endian);
> -
>    bp = &(((dsl_dataset_phys_t *) DN_BONUS (&mdn->dn))->ds_bp);
>    err = zio_read (bp, mdn->endian, &osp, &ospsize, data);
>    if (err)
> @@ -1558,7 +1569,6 @@ dnode_get_fullpath (const char *fullpath, dnode_end_t * mdn,
>        grub_dprintf ("zfs", "fsname = '%s' snapname='%s' filename = '%s'\n", 
>  		    fsname, snapname, filename);
>      }
> -  grub_dprintf ("zfs", "alive\n");
>    err = get_filesystem_dnode (&(data->mos), fsname, dn, data);
>    if (err)
>      {
> @@ -1567,12 +1577,8 @@ dnode_get_fullpath (const char *fullpath, dnode_end_t * mdn,
>        return err;
>      }
>  
> -  grub_dprintf ("zfs", "alive\n");
> -
>    headobj = grub_zfs_to_cpu64 (((dsl_dir_phys_t *) DN_BONUS (&dn->dn))->dd_head_dataset_obj, dn->endian);
>  
> -  grub_dprintf ("zfs", "endian = %d\n", mdn->endian);
> -
>    err = dnode_get (&(data->mos), headobj, DMU_OT_DSL_DATASET, mdn, data);
>    if (err)
>      {
> @@ -1580,7 +1586,6 @@ dnode_get_fullpath (const char *fullpath, dnode_end_t * mdn,
>        grub_free (snapname);
>        return err;
>      }
> -  grub_dprintf ("zfs", "endian = %d\n", mdn->endian);
>  
>    if (snapname)
>      {
> @@ -1606,8 +1611,6 @@ dnode_get_fullpath (const char *fullpath, dnode_end_t * mdn,
>      *mdnobj = headobj;
>  
>    make_mdn (mdn, data);
> -  
> -  grub_dprintf ("zfs", "endian = %d\n", mdn->endian);
>  
>    if (*isfs)
>      {
> @@ -1864,11 +1867,9 @@ zfs_fetch_nvlist (struct grub_zfs_data * data, char **nvlist)
>  static grub_err_t
>  check_pool_label (struct grub_zfs_data *data)
>  {
> -  grub_uint64_t pool_state, txg = 0;
> -  char *nvlist;
> -#if 0
> -  char *nv;
> -#endif
> +  grub_uint64_t pool_state;
> +  char *nvlist;			// for the pool
> +  char *vdevnvlist;		// for the vdev
>    grub_uint64_t diskguid;
>    grub_uint64_t version;
>    int found;
> @@ -1878,8 +1879,6 @@ check_pool_label (struct grub_zfs_data *data)
>    if (err)
>      return err;
>  
> -  grub_dprintf ("zfs", "check 2 passed\n");
> -
>    found = grub_zfs_nvlist_lookup_uint64 (nvlist, ZPOOL_CONFIG_POOL_STATE,
>  					 &pool_state);
>    if (! found)
> @@ -1889,16 +1888,16 @@ check_pool_label (struct grub_zfs_data *data)
>  	grub_error (GRUB_ERR_BAD_FS, ZPOOL_CONFIG_POOL_STATE " not found");
>        return grub_errno;
>      }
> -  grub_dprintf ("zfs", "check 3 passed\n");
>  
>    if (pool_state == POOL_STATE_DESTROYED)
>      {
>        grub_free (nvlist);
>        return grub_error (GRUB_ERR_BAD_FS, "zpool is marked as destroyed");
>      }
> -  grub_dprintf ("zfs", "check 4 passed\n");
>  
> -  found = grub_zfs_nvlist_lookup_uint64 (nvlist, ZPOOL_CONFIG_POOL_TXG, &txg);
> +  data->label_txg = 0;
> +  found = grub_zfs_nvlist_lookup_uint64 (nvlist, ZPOOL_CONFIG_POOL_TXG,
> +				   	&data->label_txg);
>    if (!found)
>      {
>        grub_free (nvlist);
> @@ -1906,15 +1905,13 @@ check_pool_label (struct grub_zfs_data *data)
>  	grub_error (GRUB_ERR_BAD_FS, ZPOOL_CONFIG_POOL_TXG " not found");
>        return grub_errno;
>      }
> -  grub_dprintf ("zfs", "check 6 passed\n");
>  
>    /* not an active device */
> -  if (txg == 0)
> +  if (data->label_txg == 0)
>      {
>        grub_free (nvlist);
>        return grub_error (GRUB_ERR_BAD_FS, "zpool isn't active");
>      }
> -  grub_dprintf ("zfs", "check 7 passed\n");
>  
>    found = grub_zfs_nvlist_lookup_uint64 (nvlist, ZPOOL_CONFIG_VERSION,
>  					 &version);
> @@ -1925,42 +1922,79 @@ check_pool_label (struct grub_zfs_data *data)
>  	grub_error (GRUB_ERR_BAD_FS, ZPOOL_CONFIG_VERSION " not found");
>        return grub_errno;
>      }
> -  grub_dprintf ("zfs", "check 8 passed\n");
>  
>    if (version > SPA_VERSION)
>      {
>        grub_free (nvlist);
>        return grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET,
> -			 "too new version %llu > %llu",
> +			 "SPA version too new %llu > %llu",
>  			 (unsigned long long) version,
>  			 (unsigned long long) SPA_VERSION);
>      }
> -  grub_dprintf ("zfs", "check 9 passed\n");
> -#if 0
> -  if (nvlist_lookup_value (nvlist, ZPOOL_CONFIG_VDEV_TREE, &nv,
> -			   DATA_TYPE_NVLIST, NULL))
> +
> +  vdevnvlist = grub_zfs_nvlist_lookup_nvlist (nvlist, ZPOOL_CONFIG_VDEV_TREE);
> +  if (!vdevnvlist)
>      {
> -      grub_free (vdev);
> -      return (GRUB_ERR_BAD_FS);
> +      grub_free (nvlist);
> +      if (!grub_errno)
> +	grub_error (GRUB_ERR_BAD_FS, ZPOOL_CONFIG_VDEV_TREE " not found");
> +      return grub_errno;
> +    }
> +
> +  found = grub_zfs_nvlist_lookup_uint64 (vdevnvlist, ZPOOL_CONFIG_ASHIFT,
> +					 &data->vdev_ashift);
> +  grub_free (vdevnvlist);
> +  if (!found)
> +    {
> +      grub_free (nvlist);
> +      if (!grub_errno)
> +	grub_error (GRUB_ERR_BAD_FS, ZPOOL_CONFIG_ASHIFT " not found");
> +      return grub_errno;
>      }
> -  grub_dprintf ("zfs", "check 10 passed\n");
> -#endif
>  
>    found = grub_zfs_nvlist_lookup_uint64 (nvlist, ZPOOL_CONFIG_GUID, &diskguid);
>    if (! found)
>      {
>        grub_free (nvlist);
>        if (! grub_errno)
> -	grub_error (GRUB_ERR_BAD_FS, ZPOOL_CONFIG_GUID " not found");
> +        grub_error (GRUB_ERR_BAD_FS, ZPOOL_CONFIG_GUID " not found");
> +      return grub_errno;
> +    }
> +
> +  found = grub_zfs_nvlist_lookup_uint64 (nvlist, ZPOOL_CONFIG_POOL_GUID, &data->pool_guid);
> +  if (! found)
> +    {
> +      grub_free (nvlist);
> +      if (! grub_errno)
> +        grub_error (GRUB_ERR_BAD_FS, ZPOOL_CONFIG_POOL_GUID " not found");
>        return grub_errno;
>      }
> -  grub_dprintf ("zfs", "check 11 passed\n");
>  
>    grub_free (nvlist);
>  
> +  grub_dprintf ("zfs", "ZFS Pool GUID: %llu (%016llx) Label: GUID: %llu (%016llx), txg: %llu, SPA v%llu, ashift: %llu\n",
> +	(unsigned long long) data->pool_guid,
> +    (unsigned long long) data->pool_guid,
> +	(unsigned long long) diskguid,
> +    (unsigned long long) diskguid,
> +    (unsigned long long) data->label_txg,
> +    (unsigned long long) version,
> +    (unsigned long long) data->vdev_ashift);
> +
>    return GRUB_ERR_NONE;
>  }
>  
> +/*
> + * vdev_label_start returns the physical disk offset (in bytes) of
> + * label "l".
> + */
> +static grub_uint64_t vdev_label_start (grub_uint64_t psize, int l)
> +{
> +  return (l * sizeof (vdev_label_t) + (l < VDEV_LABELS / 2 ?
> +				       0 : psize -
> +				       VDEV_LABELS * sizeof (vdev_label_t)));
> +}
> +
>  static void
>  zfs_unmount (struct grub_zfs_data *data)
>  {
> @@ -1979,13 +2013,13 @@ static struct grub_zfs_data *
>  zfs_mount (grub_device_t dev)
>  {
>    struct grub_zfs_data *data = 0;
> -  int label = 0;
> -  uberblock_phys_t *ub_array, *ubbest = NULL;
> -  vdev_boot_header_t *bh;
> +  int label = 0, bestlabel = -1;
> +  char *ub_array;
> +  uberblock_t *ubbest;
> +  uberblock_t *ubcur = NULL;
>    void *osp = 0;
>    grub_size_t ospsize;
>    grub_err_t err;
> -  int vdevnum;
>  
>    if (! dev->disk)
>      {
> @@ -1997,11 +2031,6 @@ zfs_mount (grub_device_t dev)
>    if (!data)
>      return 0;
>    grub_memset (data, 0, sizeof (*data));
> -#if 0
> -  /* if it's our first time here, zero the best uberblock out */
> -  if (data->best_drive == 0 && data->best_part == 0 && find_best_root)
> -    grub_memset (&current_uberblock, 0, sizeof (uberblock_t));
> -#endif
>  
>    data->disk = dev->disk;
>  
> @@ -2012,100 +2041,129 @@ zfs_mount (grub_device_t dev)
>        return 0;
>      }
>  
> -  bh = grub_malloc (VDEV_BOOT_HEADER_SIZE);
> -  if (!bh)
> +  ubbest = grub_malloc (sizeof (*ubbest));
> +  if (!ubbest)
>      {
>        zfs_unmount (data);
> -      grub_free (ub_array);
>        return 0;
>      }
> +  grub_memset (ubbest, 0, sizeof (*ubbest));
>  
> -  vdevnum = VDEV_LABELS;
> +  // Establish some constants for where things are on the device:
>  
> -  /* Don't check back labels on CDROM.  */
> -  if (grub_disk_get_size (dev->disk) == GRUB_DISK_SIZE_UNKNOWN)
> -    vdevnum = VDEV_LABELS / 2;
> +  /*
> +   * some eltorito stacks don't give us a size and
> +   * we end up setting the size to MAXUINT, further
> +   * some of these devices stop working once a single
> +   * read past the end has been issued. Checking
> +   * for a maximum part_length and skipping the backup
> +   * labels at the end of the slice/partition/device
> +   * avoids breaking down on such devices.
> +   */
> +  const int vdevnum =
> +    grub_disk_get_size (dev->disk) == GRUB_DISK_SIZE_UNKNOWN ?
> +    VDEV_LABELS / 2 : VDEV_LABELS;
> +
> +  // Size in bytes of the device (disk or partition) aligned to label size
> +  grub_uint64_t device_size = 
> +	grub_disk_get_size (dev->disk) << GRUB_DISK_SECTOR_BITS;
> +  const grub_uint64_t alignedbytes =
> +    P2ALIGN (device_size, (grub_uint64_t) sizeof (vdev_label_t));
This is better to be moved to vdev_label_start.
>  
> -  for (label = 0; ubbest == NULL && label < vdevnum; label++)
> +  for (label = 0; label < vdevnum; label++)
>      {
> -      grub_zfs_endian_t ub_endian = UNKNOWN_ENDIAN;
> -      grub_dprintf ("zfs", "label %d\n", label);
> +      grub_uint64_t labelstartbytes = vdev_label_start (alignedbytes, label);
> +      grub_uint64_t labelstart = labelstartbytes >> GRUB_DISK_SECTOR_BITS;
>  
> -      data->vdev_phys_sector
> -	= label * (sizeof (vdev_label_t) >> SPA_MINBLOCKSHIFT)
> -	+ ((VDEV_SKIP_SIZE + VDEV_BOOT_HEADER_SIZE) >> SPA_MINBLOCKSHIFT)
> -	+ (label < VDEV_LABELS / 2 ? 0 : grub_disk_get_size (dev->disk)
> -	   - VDEV_LABELS * (sizeof (vdev_label_t) >> SPA_MINBLOCKSHIFT));
> +      grub_dprintf ("zfs", "reading label %d at sector %llu (byte %llu)\n",
> +		    label, (unsigned long long) labelstart, 
> +		(unsigned long long) labelstartbytes);
>  
> -      /* Read in the uberblock ring (128K). */
> -      err = grub_disk_read (data->disk, data->vdev_phys_sector
> -			    + (VDEV_PHYS_SIZE >> SPA_MINBLOCKSHIFT),
> -			    0, VDEV_UBERBLOCK_RING, (char *) ub_array);
> +      data->vdev_phys_sector = labelstart +
> +		((VDEV_SKIP_SIZE + VDEV_BOOT_HEADER_SIZE) >> GRUB_DISK_SECTOR_BITS);
> +
> +      err = check_pool_label (data);
>        if (err)
> -	{
> -	  grub_errno = GRUB_ERR_NONE;
> -	  continue;
> -	}
> -      grub_dprintf ("zfs", "label ok %d\n", label);
> +	    {
> +		  grub_dprintf ("zfs", "error checking label %d\n", label);
> +	      grub_errno = GRUB_ERR_NONE;
> +	      continue;
> +	    }
>  
> -      ubbest = find_bestub (ub_array, data->vdev_phys_sector);
> -      if (!ubbest)
> -	{
> -	  grub_dprintf ("zfs", "No uberblock found\n");
> -	  grub_errno = GRUB_ERR_NONE;
> -	  continue;
> -	}
> -      ub_endian = (grub_zfs_to_cpu64 (ubbest->ubp_uberblock.ub_magic, 
> -				     LITTLE_ENDIAN) == UBERBLOCK_MAGIC 
> -		   ? LITTLE_ENDIAN : BIG_ENDIAN);
> -      err = zio_read (&ubbest->ubp_uberblock.ub_rootbp, 
> -		      ub_endian,
> -		      &osp, &ospsize, data);
> +      /* Read in the uberblock ring (128K). */
> +      err = grub_disk_read (data->disk,
> +			    data->vdev_phys_sector +
> +			    (VDEV_PHYS_SIZE >> GRUB_DISK_SECTOR_BITS),
> +			    0, VDEV_UBERBLOCK_RING, ub_array);
>        if (err)
> -	{
> -	  grub_dprintf ("zfs", "couldn't zio_read\n"); 
> -	  grub_errno = GRUB_ERR_NONE;
> -	  continue;
> -	}
> +	    {
> +		  grub_dprintf ("zfs", "error reading uberblock ring for label %d\n", label);
> +	      grub_errno = GRUB_ERR_NONE;
> +	      continue;
> +	    }
>  
> -      if (ospsize < OBJSET_PHYS_SIZE_V14)
> -	{
> -	  grub_dprintf ("zfs", "osp too small\n"); 
> -	  grub_free (osp);
> -	  continue;
> -	}
> -      grub_dprintf ("zfs", "ubbest %p\n", ubbest);
> +      ubcur = find_bestub (ub_array, data);
> +      if (!ubcur)
> +	    {
> +	      grub_dprintf ("zfs", "No good uberblocks found in label %d\n", label);
> +	      grub_errno = GRUB_ERR_NONE;
> +	      continue;
> +	    }
>  
> -      err = check_pool_label (data);
> -      if (err)
> +      if (vdev_uberblock_compare (ubcur, ubbest) > 0)
> +	    {
> +		  // Looks like the block is good, so use it.
> +	      grub_memcpy (ubbest, ubcur, sizeof (*ubbest));
> +	      bestlabel = label;
> +	      grub_dprintf ("zfs", "Current best uberblock found in label %d\n", label);
> +	    }
> +    }
> +  grub_free (ub_array);
> +
> +  // We zero'd the structure to begin with.  If we never assigned to it, 
> +  // magic will still be zero.
> +  if (!ubbest->ub_magic)
> +    {
> +      grub_error (GRUB_ERR_BAD_FS, "couldn't find a valid ZFS label");
> +      zfs_unmount (data);
> +      grub_free (ubbest);
> +      return 0;
> +    }
> +
> +  grub_dprintf ("zfs", "ubbest %p in label %d\n", ubbest, bestlabel);
> +
> +  grub_zfs_endian_t ub_endian =
> +    grub_zfs_to_cpu64 (ubbest->ub_magic, LITTLE_ENDIAN) == UBERBLOCK_MAGIC
> +    ? LITTLE_ENDIAN : BIG_ENDIAN;
> +  err = zio_read (&ubbest->ub_rootbp, ub_endian, &osp, &ospsize, data);
> +
> +  if (err)
>  	{
> -	  grub_errno = GRUB_ERR_NONE;
> -	  continue;
> +	  grub_error (GRUB_ERR_BAD_FS, "couldn't zio_read object directory");
> +	  zfs_unmount (data);
> +	  grub_free (ubbest);
> +	  return 0;
>  	}
> -#if 0
> -      if (find_best_root &&
> -	  vdev_uberblock_compare (&ubbest->ubp_uberblock,
> -				  &(current_uberblock)) <= 0)
> -	continue;
> -#endif
> -      /* Got the MOS. Save it at the memory addr MOS. */
> -      grub_memmove (&(data->mos.dn), &((objset_phys_t *) osp)->os_meta_dnode,
> -		    DNODE_SIZE);
> -      data->mos.endian = (grub_zfs_to_cpu64 (ubbest->ubp_uberblock.ub_rootbp.blk_prop, ub_endian) >> 63) & 1;
> -      grub_memmove (&(data->current_uberblock),
> -		    &ubbest->ubp_uberblock, sizeof (uberblock_t));
> -      grub_free (ub_array);
> -      grub_free (bh);
> -      grub_free (osp);
> -      return data;  
> +
> +  if (ospsize < OBJSET_PHYS_SIZE_V14)
> +    {
> +	  grub_error (GRUB_ERR_BAD_FS, "osp too small");
> +	  zfs_unmount (data);
> +	  grub_free (osp);
> +	  grub_free (ubbest);
> +	  return 0;
>      }
> -  grub_error (GRUB_ERR_BAD_FS, "couldn't find a valid label");
> -  zfs_unmount (data);
> -  grub_free (ub_array);
> -  grub_free (bh);
> -  grub_free (osp);
>  
> -  return 0;
> +  /* Got the MOS. Save it at the memory addr MOS. */
> +  grub_memmove (&(data->mos.dn), &((objset_phys_t *) osp)->os_meta_dnode, DNODE_SIZE);
> +  data->mos.endian =
> +		(grub_zfs_to_cpu64 (ubbest->ub_rootbp.blk_prop, ub_endian) >> 63) & 1;
> +  grub_memmove (&(data->current_uberblock), ubbest, sizeof (uberblock_t));
> +
> +  grub_free (osp);
> +  grub_free (ubbest);
> +  
> +  return (data);
>  }
>  
>  grub_err_t
> @@ -2149,33 +2207,18 @@ zfs_label (grub_device_t device, char **label)
>  static grub_err_t 
>  zfs_uuid (grub_device_t device, char **uuid)
>  {
> -  char *nvlist;
> -  int found;
>    struct grub_zfs_data *data;
> -  grub_uint64_t guid;
> -  grub_err_t err;
> -
> -  *uuid = 0;
>  
>    data = zfs_mount (device);
>    if (! data)
>      return grub_errno;
>  
> -  err = zfs_fetch_nvlist (data, &nvlist);
> -  if (err)
> -    {
> -      zfs_unmount (data);
> -      return err;
> -    }
> -
> -  found = grub_zfs_nvlist_lookup_uint64 (nvlist, ZPOOL_CONFIG_POOL_GUID, &guid);
> -  if (! found)
> -    return grub_errno;
> -  grub_free (nvlist);
> -  *uuid = grub_xasprintf ("%016llx", (long long unsigned) guid);
> +  *uuid = grub_xasprintf ("%016llx", (long long unsigned) data->pool_guid);
>    zfs_unmount (data);
> +
>    if (! *uuid)
>      return grub_errno;
> +
>    return GRUB_ERR_NONE;
>  }
>  
> diff --git a/include/grub/zfs/uberblock_impl.h b/include/grub/zfs/uberblock_impl.h
> index 1bf7f2b..94b5ad7 100644
> --- a/include/grub/zfs/uberblock_impl.h
> +++ b/include/grub/zfs/uberblock_impl.h
> @@ -23,6 +23,8 @@
>  #ifndef _SYS_UBERBLOCK_IMPL_H
>  #define	_SYS_UBERBLOCK_IMPL_H
>  
> +#define UBMAX(a,b) ((a) > (b) ? (a) : (b))
> +
>  /*
>   * The uberblock version is incremented whenever an incompatible on-disk
>   * format change is made to the SPA, DMU, or ZAP.
> @@ -45,16 +47,10 @@ typedef struct uberblock {
>  	blkptr_t	ub_rootbp;	/* MOS objset_phys_t		*/
>  } uberblock_t;
>  
> -#define	UBERBLOCK_SIZE		(1ULL << UBERBLOCK_SHIFT)
> -#define	VDEV_UBERBLOCK_SHIFT	UBERBLOCK_SHIFT
> -
> -/* XXX Uberblock_phys_t is no longer in the kernel zfs */
> -typedef struct uberblock_phys {
> -	uberblock_t	ubp_uberblock;
> -	char		ubp_pad[UBERBLOCK_SIZE - sizeof (uberblock_t) -
> -				sizeof (zio_eck_t)];
> -	zio_eck_t	ubp_zec;
> -} uberblock_phys_t;
> -
> +#define	VDEV_UBERBLOCK_SHIFT(as)	UBMAX(as, UBERBLOCK_SHIFT)
> +#define	UBERBLOCK_SIZE(as)		(1ULL << VDEV_UBERBLOCK_SHIFT(as))
> + 
> +// Number of uberblocks that can fit in the ring at a given ashift
> +#define UBERBLOCK_COUNT(as) (VDEV_UBERBLOCK_RING >> VDEV_UBERBLOCK_SHIFT(as))
>  
>  #endif	/* _SYS_UBERBLOCK_IMPL_H */
>
>
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel


-- 
Regards
Vladimir 'φ-coder/phcoder' Serbinenko



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 294 bytes --]