[illumos-Developer] iSCSI UNMAP with DKIOCFREE

[George Wilson]

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

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.

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.

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.

objset_t *os = zv->zv_objset;

rl = zfs_range_lock(&zv->zv_znode, df.df_start, df.df_length, RL_WRITER);
tx = dmu_tx_create(zv->zv_objset);
error = dmu_tx_assign(tx, TXG_WAIT);
if (error) {
	dmu_tx_abort(tx);
	break;
}

zvol_log_truncate(zv, tx, df.df_start, df.df_length, B_TRUE);
error = dmu_free_long_range(os, ZVOL_OBJ, df.df_start, df.df_length);
dmu_tx_commit(tx);
zfs_range_unlock(rl);
zil_commit(zv->zv_zilog, ZVOL_OBJ);

if (error == 0&&  (df.df_flags&  DF_WAIT_SYNC))
	txg_wait_synced(dmu_objset_pool(os), 0);

break;


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

Thanks,
George

On 2/23/11 5:53 PM, Dan McDonald wrote:
> Smoke test blew up, THEN I read mail...
>
> On Wed, Feb 23, 2011 at 03:37:12PM -0800, George Wilson wrote:
>> More comments:
>>
>> zvol.c:
>>
>> You're going to have to change the flow the if/else block to
>> accommodate some of these comments:
>>
>> line# 1532 - you should be able to just set this to ZVOL_OBJ
>> after line# 1690 - you need to do dmu_tx_commit()
>> after line# 1694 - you need to a zil_commit() but only if error != 0
>>
>> line # 407 - you need to define a new replay op
> Fixed, fixed, fixed, and fixed.
>
> Webrev's updated:
>
> 	http://www.kebe.com/~danmcd/webrevs/unmap/
>
> I'm now gonna reboot and re-smoke-test.  If I find more, I'll update (so try
> reload before sending mail, just in case... ;).
>
> Thanks for your time&  patience,
> Dan
>
> _______________________________________________
> Developer mailing list
> Developer at lists.illumos.org
> http://lists.illumos.org/m/listinfo/developer