[ZoL#22] Implement DKIOCFLUSHWRITECACHE vdev ioctl command #3
Conversation
lib/libzpool/vdev_disk_aio.c
Outdated
| if (errno == EINVAL || errno == ENOTTY) { | ||
| fprintf(stderr, "Disk %s does not support synchronize " | ||
| "cache SCSI command\n", vd->vdev_path); | ||
| vda->vda_noflush = B_TRUE; |
There was a problem hiding this comment.
Can dump the sense data in case of failure? Depending on the failed mode -- we might as well disable flushing altogether for this device for future IOs?
There was a problem hiding this comment.
I try to distinguish between two cases:
- cache flush not possible
* this is either EINVAL if SG_IO is not supported at all (i.e. lofi device)
* or ENOTTY if SG_IO is understood nevertheless cache synchronize command not implemented - transient error (all other cases)
For case number 1 we never attempt to flush again in the future (stop losing time). For case number 2 we return error for that particular zio, but cache flushing remains enabled.
Regarding printing sense data, there is a bug in current fix as I need to distinguish between failed ioctl and failed SCSI command. For failed SCSI command we could print the sense data to stderr. I would prefer to print it raw (in hex), the routine for decoding sense data would be pretty long (example: https://fossies.org/dox/sg3_utils-1.42/sg__lib_8c_source.html#l01412) . So everytime when ioctl succeeds but SCSI cmd fails we will print an error to stderr. I just hope it won't happen too frequently :-)
Let me know if you agree with the proposal above. thanks
|
Hi @vishnuitta , I don't want to look that I always have some excuse for writing tests, but it is rather complicated in this case :) Higher level code will never know that a flush has failed with the test disks which we are using. Flush is not supported for them, thus flush is noop and not reported to higher level code. and we don't have real SCSI devices in automated tests and even if we had, I would not be able to simulate transient failures on them which is the only case when zio actually fails. I fear we have to trust the manual testing I have done and hope it will not break in future. |
jkryl
force-pushed
the
disk-flush
branch
2 times, most recently
from
c47c918 to
e29765e
Compare
|
does this have dependency on the kernel or other libraries @jkryl ? |
|
@vishnuitta: talking only about the code I have added, the only dependency is on sgio header file which contains definitions needed for SG_IO ioctl to kernel. SG_IO ioctl has been standard part of linux kernel for a long time. So we don't depend on anything unusual. |
|
Changes looks good @jkryl |
lib/libzpool/vdev_disk_aio.c
Outdated
| @@ -404,6 +408,95 @@ kick_submitter(vdev_disk_aio_t *vda) | |||
| assert(rc == sizeof (data)); | |||
| } | |||
|
|
|||
| #define SCSI_STATUS_GOOD 0 | |||
| #define SCSI_STATUS_CHECK_COND 2 | |||
There was a problem hiding this comment.
should we get these constants from some standard header files?
There was a problem hiding this comment.
any standard header files with these constants @jkryl ?
There was a problem hiding this comment.
Thanks Vitta! I found most of them defined in scsi/scsi.h linux header file. Only two defines remained and those have been moved to the beginning of the file to usual place.
jkryl
force-pushed
the
disk-flush
branch
3 times, most recently
from
71547a6 to
5b98569
Compare
|
The ticket for documentation issue has been created (zenhub ticket #46). In addition to that I have mentioned it in README file. |
CCS-54 google test framework for uZFS
As noted in the code, this flush command works only for SCSI drives controlled by sd driver and regarding synchronicity of flush command, I tested performance with synchronous and asynchronous (using taskq) flush and with sync workloads the performance penalty is 25% when using async and only 15% when using sync. So we will be calling flush synchronously in zio pipeline. So just to highlight it, for drives which support SCSI SYNCHRONIZE CACHE command we sacrifice 15% of performance for sync workloads, but that is understandable as we have more work to do in such case.