[illumos-Developer] Review --> iSCSI UNMAP support

[Garrett D'Amore]

On Fri, 2011-02-11 at 10:57 -0500, Albert Lee wrote:
> On Thu, Feb 10, 2011 at 12:10 AM, Dan McDonald <danmcd at nexenta.com> wrote:
> > On Wed, Feb 09, 2011 at 10:07:14PM -0500, Eric Schrock wrote:
> >> Can you explain why unmap is only supported for zvols without a
> >> refreservation?
> >
> > No, I can't.  I took this existing code and dropped it in.  I took no part in
> > its design.  You know my background isn't in ZFS, so I'm willing to accept
> > any clues here, even if I can't act upon them immediately.
> >
> >> Freeing this data
> >
> > You mean dmu_free_long_range(), right?
> >
> >> can have meaningful impact on future operations (for example, subsequent
> >> snapshots, or changing the zvol to be sparse).  Seems like extra work and
> >> causes more harm than good.
> >
> > Interesting.  I believe the code forces the zvol to a minimum of zle
> > compression (see the do_unmap_setup() function).  Wouldn't this allow the
> > dmu_free_long_range() to be at least space-efficient?
> 
> Is zle really necessary for the freed region to be reused by other
> objects? Should we check for SPA_VERSION_ZLE_COMPRESSION?

That's actually a good point I think.  I'm not sure that ZLE is
necessary here, although for future work for ZFS initiated unmap I think
we will want to make use of ZLE and in particular we will want to hook
onto its calls release space.  But that's not this change.

Sumit? What was the motivation of using ZLE here?

As far as checking for SPA_VERSION_ZLE_COMPRESSION, my read of the code
is that the ZFS SET_PROP ioctl will fail if the underlying SPA doesn't
support it, thanks to explicit checks for ZFS_PROP_COMPRESSION in
zfs_check_settable():

http://src.illumos.org/source/xref/illumos-opengrok/usr/src/uts/common/fs/zfs/zfs_ioctl.c#3340

(which is called by the ioctl handler.)

So while the question of whether ZLE compression needs to be *required*
for this feature remains, an explicit check for it doesn't seem to be
needed.

	- Garrett

> -Albert
> 
> _______________________________________________
> Developer mailing list
> Developer at lists.illumos.org
> http://lists.illumos.org/m/listinfo/developer