RE: [bug #48885] zfs_mount fails with `org.zfsonlinux:large_dnode` enabled: Unsupported features in pool

[Bass, Ned]

> -----Original Message-----
> From: Toomas Soome [mailto:address@hidden
> Sent: Wednesday, August 24, 2016 2:26 PM
> To: Andrei Borzenkov
> Cc: Behlendorf, Brian D.; The development of GNU GRUB; Bass, Ned
> Subject: Re: [bug #48885] zfs_mount fails with `org.zfsonlinux:large_dnode`
> enabled: Unsupported features in pool
> 
> 
> > On 25. aug 2016, at 0:04, Andrei Borzenkov <address@hidden> wrote:
> >
> > 24.08.2016 23:10, Brian Behlendorf пишет:
> >> Thomas,
> >>
> >> You should be able to simply add 'org.zfsonlinux:large_dnode' to the
> >> list of supported features.  As long as the dataset property dnodesize
> >> is set to legacy there won't be any on-disk format changes.  The dnodes
> >> on disk will still be 512 bytes in size and it will just leverage some
> >> previously unused pad space in the dnode_phys_t.
> >>
> >> Since we were concerned about grub support the zfs command now prohibits
> >> users from setting the dnodesize property to a non-legacy value for
> >> datasets with the bootfs property set.
> >>
> >
> > We have no way to restrict which filesystems users will access at boot
> > time. Can we check value of this property in grub?
> 
> yes, this is exactly why the features for read list exist - if the pool is 
> adding entry
> there and boot loader does not have matching entry in its implementation, you
> deny the access to pool. Which is exactly what did happen for this user.

Exactly, the large_dnode feature is active on the pool in the bug report, so 
the read access was correctly denied. Unfortunately this doesn't distinguish 
which datasets are using the feature, so if any dataset is using the feature 
grub-probe will fail. We could add special handling to check if the particular 
dataset in question uses the feature, but I agree it would be best to simply 
add full support for the feature. At minimum, that would involve:

- Update any sites that use the DNODE_SIZE macro to dynamically compute a size 
based on the dn_extra_slots field of the dnode_phys_t. i.e. 
((dn->dn_extra_slots + 1) << DNODE_SHIFT)
- Update any use of dn.dn_spill to use the new DN_SPILL_BLKPTR(dnp) macro.
- Several structs in grub-core/fs/zfs/zfs.c have an embedded dnode_phys_t. 
These may need to be refactored to allow for variable sizing.

Like Brian, I'd also be happy to clarify anything or review any patches.

Thanks,
Ned