7598 loader: Implement disk_ioctl() to support DIOCGSECTORSIZE and DIOCGMEDIASIZE.

Review Request #271 — Created Nov. 20, 2016 and submitted

tsoome
illumos-gate
7598
282
272
8047917...
general

7598 loader: Implement disk_ioctl() to support DIOCGSECTORSIZE and DIOCGMEDIASIZE.

Need interface to extract information about disk abstraction,
to read disk or partition size depending on the provided argument
and adjust disk size based on information in partition table.

The disk handle from disk_open() has d_offset field to point to
partition start. So we can use this fact to return either whole disk
size or partition size. For this we only need to record partition size
we get from disk_open() anyhow.

In addition, this will also make it possible to adjust the disk media size
based on information from partition table. The problem with disk size is
about some BIOS systems reporting bogus disk size for 2+TB disks, but
since such disks are using GPT partitioning, and GPT does have information
about disk size (alternate LBA + 1), we can use this fact to record disk
size based on partition table.

This patch does exactly this: implements DIOCGSECTORSIZE and DIOCGMEDIASIZE
ioctl, and DIOCGMEDIASIZE will report either disk media size or partition size.

Adds ptable_getsize() call to read partition size in bytes from ptable pointer.
Updates disk_open() to use ptable_getsize() to update mediasize value.

Implements GPT detection function to update ptable size (used by
ptable_getsize()) according to alternate lba (which is location of backup copy
of GPT header table).

This implementation is used by https://reviews.freebsd.org/D8581
and https://www.illumos.org/rb/r/272/



  • 0
  • 0
  • 6
  • 1
  • 7
Description From Last Updated
tsoome
tsoome
rm
  1. 
      
  2. usr/src/boot/sys/boot/common/disk.c (Diff revision 1)
     
     
    Shouldn't this be checking for overflow?
    1. off_t on bios loader is 8 bytes (64bit). So entrysize is either smaller (from mbr/vtoc etc - those are using smaller values for partition entries), or 64bit value from GPT table, which should be valid as ptable_open() had to be successful, otherwise we would not be there. So I think we can assume the value is valid and we wont overflow.

    2. Where does ptable_open() actually check that the sectors times the sector size doesn't overflow? For example, the ZFS code doesn't check before calling in.

    3. well, right, it does not check if partition size * sector size does not overflow uint64_t.

  3. usr/src/boot/sys/boot/common/part.c (Diff revision 1)
     
     
    Can you explain why we reset it? This looks like we're updating the maximum as opposed to resetting it (which I would think of as setting to some default value like zero to start some operation over again).
    1. Ah, the wording is perhaps bad. The idea is that if we have situation where int13 did lie about disk size (and the value from int13 was passed to ptable_open() and got recorded as table->sectors), now as we just did successfully read and verified GPT table, GPT has disk size recorded in header - in form of alt_lba number, which is the location of backup header, and it always is the last sector of the disk. So it does give us an option to verify and fix the disk size based on information from the GPT. This information is valid as the gpt code did verify primary and backup headers, so we can trust it, and adjust the disk size recorded earlier. Since the disk size information seems to be problem only with >2TB disks, other partition table types are not usable with those, so this update is only implemented for case of GPT.

      So, in that sense, we are resetting this value to default value - to actual disk size. For case the GPT is constructed on image and copied to physical disk/stick, the alt_lba can actually not point to the last sector of the disk, thats why I only update the size if the table->sectors is actually smaller value.

    2. I think it'd be useful to get this kind of information into the comment.
    3. Did add the comment.

  4. usr/src/boot/sys/boot/common/part.c (Diff revision 1)
     
     
    How do we guarantee that this doesn't overflow?
    1. same as on first case; the non-gpt partition tables have smaller values, so we will fit into 64bit, and gpt should be valid or we are not there...

    2. If we want to always rely on that, I guess. But see prior comment that I'm not sure it'll always be safe to do so.
    3. Implemented the check.

  5. 
      
tsoome
tsoome
rm
  1. 
      
  2. usr/src/boot/sys/boot/common/part.c (Diff revision 3)
     
     

    I think this could use a small amount of grammar clean up. How about:

     * If the disk's sector count is smaller than the sector count recorded
     * in the disk's GPT table header, set table->sectors to the value
     * recorded in GPT tables.  This is done to work around buggy firmware
     * that returns truncated disk sizes.
     *
     * Note, this is still not a foolproof way to get the disk's size. For
     * example, an image file can be truncated when copied to smaller media.
    
  3. usr/src/boot/sys/boot/common/part.c (Diff revision 3)
     
     

    Any reason not to have this take a pointer for the output size and then return an int with an error code? This way you don't have to do the set to zero games.

  4. usr/src/boot/sys/boot/common/part.c (Diff revision 3)
     
     
    I think EOVERFLOW would probably be better here.
  5. 
      
tsoome
rm
  1. Ship It!
  2. 
      
tsoome
tsoome
rm
  1. 
      
  2. usr/src/boot/sys/boot/common/disk.c (Diff revisions 4 - 5)
     
     
    If we're changing this to be more consistent, don't we need an update to the boot/sys/sys/disk.h header to actually say that the ioctl takes a uint64_t?
    1. pushed it to https://www.illumos.org/rb/r/282/ (7641 loader: disk/part api needs to use uint64_t offsets)

  3. 
      
tsoome
rm
  1. Ship It!
  2. 
      
otis
  1. Ship It!
  2. 
      
tsoome
Review request changed

Status: Closed (submitted)

Loading...