[illumos-Developer] iSCSI UNMAP with DKIOCFREE

[Dan McDonald]

On Fri, Feb 25, 2011 at 08:21:45AM -0800, George Wilson wrote:
> zvol.c:
> 
> Just a nit but I would call the replay function
> 'zvol_replay_truncate' since that seems to be naming standard for zil
> logging and replay functions. The same would apply to
> zvol_log_free_long_range().

Fixed.

> Also looking at zvol_replay_truncate() I think that it's not going to
> do what we really want since it still leaves you susceptible to a
> possible crash in the middle of dmu_free_long_range(). I think
> instead you want to call VOP_IOCTL() again so that we process the
> DKIOCFREE again.

I followed the example of zvol_replay_write().  Is that code wrong as well?
(e.g. What if a crash occurs in the middle of dmu_write() in
zvol_replay_write()?)  If zvol_replay_write() is wrong also, does this mean
we should effectively be calling the zvol_write() function again?

Also, if I reinvoke VOP_IOCTL(DKIOCFREE), what about the DF_WAIT_SYNC flag?
Unless there's a way to store that context in the log as well, I'll have to
decide what to set that to during replay.  I'm inclined to *set* the flag
since the log replay will only occur during a recovery, and one would expect
extra disk scribbling during that time, no?

> line# 1568 - I think we may want to pass in a sync boolean to
> zvol_log_truncate() and set itx_sync to sync. For this usage of
> zvol_log_truncate() the sync value should be B_TRUE since we're
> treating all of the TRIM calls as synchronous.

Done.

> I restructured the code below to simplify things. In particular, the
> thing that needs to happen is the zil_commit() needed to be moved up.

Earlier you said:

	after line# 1694 - you need to a zil_commit() but only if error != 0

you must've meant the return from dmu_tx_assign(), not the return from
dmu_free_long_range().  I assumed the latter.

> We also need to test the log/replay logic. So I would recommend
> setting a breakpoint after zvol_log_truncate() and forcing a panic.
> Then make sure that we process this ioctl again on bootup. Also do
> the same thing after dmu_free_long_range().

IIRC "dtrace -w" is my friend here.

I'm very curious about whether or note zvol_replay_write() is broken as well.

Thanks,
Dan