Re: [PATCHv2] usb: gadget: storage: optional SCSI WRITE FUA bit

From: MichaÅ Nazarewicz
Date: Wed Jul 14 2010 - 10:23:15 EST


On Wed, 14 Jul 2010 15:44:58 +0200, Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote:
@@ -93,6 +93,8 @@
* removable Default false, boolean for removable media
* luns=N Default N = number of filenames, number of
* LUNs to support
+ * fua=b[,b...] Default false, booleans for ignore FUA
flag
+ * in SCSI WRITE(6,10,12) commands

I wonder if it makes sense to make it per-LUN. I would imagine
that it's great to ignore FUA if the device has its own power supply
in which case after disconnect the data won't be lost. This is a
per-device property not really per-LUN. As such I'd make this option
global for the gadget.

Make sense only for removable media with one partition.
Otherwise. why we have sync option per partition f.e., not per device?

Ah, OK, I see why this is per LUN. You want to be able not to ignore
FUA if the backing storage is a removable media, right?


+ ssize_t rc = count;

Not really needed here.

See below

This still stands. The ârcâ is not needed.

+ if (sscanf(buf, "%d", &i) != 1)
+ return -EINVAL;

Why not simple_strtol() directly?

I did it in the same way like fsg_store_ro() does.
I have no objections to back to previous solution.

OK. I'd use simpre_strol() myself. Maybe even patched fsg_store_ro().

+
+ if (curlun->fua)
+ fsg_lun_fsync_sub(curlun);

Shouldn't that read something like:

+ if (!curlun->fua && i)
+ fsg_lun_fsync_sub(curlun);

ie. there is no sense in syncing if FUA was active (in which case all
writes were synced already, right?) or if the new value is false (since
then user does not won't syncing).

The idea is to sync data before switching from async mode.

But there can be a case of switching from async to async when syncing
is not necessary. That's why I proposed the &&. With fua = 1 meaning
ignore the flag my proposal would be:

+ if (!i && curlun->fua)
+ fsg_lun_fsync_sub(curlun);

Actually fua = 1 means ignorance of that flag.

ignore_fua would be better name then I think. This also stands for
module parameter.

--
Best regards, _ _
| Humble Liege of Serenely Enlightened Majesty of o' \,=./ `o
| Computer Science, MichaÅ "mina86" Nazarewicz (o o)
+----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/