[illumos-Developer] iSCSI UNMAP with DKIOCFREE

[George Wilson]

On 2/25/11 8:40 AM, Dan McDonald wrote:
> 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?

The zvol_replay_write() will work because we're only dealing with one 
transaction. What I'm concerned about is the fact that 
dmu_free_long_range() will create multiple transactions. That said, it 
might still work since the log record will still exist on disk.

Let's leave that code the same (with the exception of the itx_sync 
change) and try the reboot experiment I mentioned. But you'll want to 
add another reboot point in zvol_replay_truncate() prior to calling 
dmu_free_long_range() and also after it. What we need to see is that 
zvol_replay_truncate() is called after these reboot points. If they are 
then we're still replaying the same log block on disk and we can leave 
the code the same. Make sense?

>> 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.

Hmm, I guess if we failed to free the blocks then we probably shouldn't 
commit the zil either. Good point.

- George
>> 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
>
> _______________________________________________
> Developer mailing list
> Developer at lists.illumos.org
> http://lists.illumos.org/m/listinfo/developer