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

[Dan McDonald]

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?

> Also, shouldn't we be able to support this for files as well (see below)?

Given F_FREESP...

> Rather than making direct ZFS calls and picking apart zvol backing stores,
> wouldn't a better architecture be to leverage F_FREESP (or some new ioctl)
> and just have zvols implement tha?  Seems like that would also work for
> file-based LUNs.

...that would make sense.

With the current code, the only way UNMAP works is on a zvol by doing
dmu_free_long_range().  I suppose one could take the DMU code in the current
sbd_zvol_unmap() and instead place it somewhere in zfs_ioctl.c, where it
would implement an ioctl on an open zvol descriptor.  Is this what you were
thinking?  Then the UNMAP handlers would be able to perform
backing-store-specific ops (FREESP for a file, your zvol IOCTL for a zvol,
etc.).

That sounds like a better long-term solution.  I do, however, have what I
have now.  Is what is there now harmful?  Or is it something that can go in
(the UNMAP handlers seem sane, and I factored-out the unmap setup code form
the original diffs to ease porting and fixes) and be replaced with a cleaner,
more general-purpose, design later?

Thanks,
Dan