Prev: emacs and "linux" coding style
Next: [PATCH v2 2/2] ux500: add ab8500-regulators machine specific data
From: Alan Stern on 13 Jul 2010 10:10
On Tue, 13 Jul 2010, Andy Shevchenko wrote:

> MS Windows mounts removable storage in "Removal optimized mode" by
> default. All the writes to the media are synchronous which is achieved
> by setting FUA (Force Unit Access) bit in SCSI WRITE(10,12) commands.
> This prevents I/O requests aggregation in block layer dramatically
> decreasing performance.
>
> This patch brings an option to accept or ignore mentioned bit
> a) via specifying module parameter "fua", or
> b) through sysfs entry
> /sys/devices/platform/musb_hdrc/gadget/gadget-lun-N/fua

....

> @@ -111,11 +113,11 @@
> * PAGE_CACHE_SIZE)
> *
> * If CONFIG_USB_FILE_STORAGE_TEST is not set, only the "file", "ro",
> - * "removable", "luns", "stall", and "cdrom" options are available; default
> + * "removable", "luns", "fua", "stall", and "cdrom" options are available; default
> * values are used for everything else.
> *
> * The pathnames of the backing files and the ro settings are available in
> - * the attribute files "file" and "ro" in the lun<n> subdirectory of the
> + * the attribute files "file", "fua", and "ro" in the lun<n> subdirectory of the
> * gadget's sysfs directory. If the "removable" option is set, writing to
> * these files will simulate ejecting/loading the medium (writing an empty
> * line means eject) and adjusting a write-enable tab. Changes to the ro

Please do not create lines longer than 80 columns. Reflow the text.

> @@ -307,6 +309,7 @@ static struct {
>
> int removable;
> int can_stall;
> + int fua;
> int cdrom;
>
> char *transport_parm;
> @@ -326,6 +329,7 @@ static struct {
> .protocol_parm = "SCSI",
> .removable = 0,
> .can_stall = 1,
> + .fua = 1,
> .cdrom = 0,
> .vendor = FSG_VENDOR_ID,
> .product = FSG_PRODUCT_ID,
> @@ -350,6 +354,9 @@ MODULE_PARM_DESC(removable, "true to simulate removable media");
> module_param_named(stall, mod_data.can_stall, bool, S_IRUGO);
> MODULE_PARM_DESC(stall, "false to prevent bulk stalls");
>
> +module_param_named(fua, mod_data.fua, bool, S_IRUGO);
> +MODULE_PARM_DESC(fua, "true to obey SCSI WRITE(6,10,12) FUA bit");
> +
> module_param_named(cdrom, mod_data.cdrom, bool, S_IRUGO);
> MODULE_PARM_DESC(cdrom, "true to emulate cdrom instead of disk");

This implementation is wrong. If "fua" is supposed to be per-lun
then the module parameter needs to accept an array of values, like the
"file" and "ro" parameters.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Michał Nazarewicz on 14 Jul 2010 08:40
On Wed, 14 Jul 2010 11:05:31 +0200, Andy Shevchenko <andy.shevchenko(a)gmail.com> wrote:
> MS Windows mounts removable storage in "Removal optimized mode" by
> default. All the writes to the media are synchronous which is achieved
> by setting FUA (Force Unit Access) bit in SCSI WRITE(10,12) commands.
> This prevents I/O requests aggregation in block layer dramatically
> decreasing performance.

> diff --git a/drivers/usb/gadget/file_storage.c b/drivers/usb/gadget/file_storage.c
> index b49d86e..45f58d9 100644
> --- a/drivers/usb/gadget/file_storage.c
> +++ b/drivers/usb/gadget/file_storage.c
> @@ -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.

> @@ -743,6 +752,24 @@ static ssize_t fsg_store_ro(struct device *dev, struct device_attribute *attr,
> return rc;
> }
>+static ssize_t fsg_store_fua(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + ssize_t rc = count;

Not really needed here.

> + struct fsg_lun *curlun = fsg_lun_from_dev(dev);
> + int i;
> +
> + if (sscanf(buf, "%d", &i) != 1)
> + return -EINVAL;

Why not simple_strtol() directly?

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

> +
> + curlun->fua = !!i;
> +
> + return rc;

Just plain:

+ return count;

> +}
> +
> static ssize_t fsg_store_file(struct device *dev, struct device_attribute *attr,
> const char *buf, size_t count)
> {


--
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(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Andy Shevchenko on 14 Jul 2010 09:50
2010/7/14 Michał Nazarewicz <m.nazarewicz(a)samsung.com>:
> On Wed, 14 Jul 2010 11:05:31 +0200, Andy Shevchenko
> <andy.shevchenko(a)gmail.com> wrote:
>>
>> MS Windows mounts removable storage in "Removal optimized mode" by
>> default. All the writes to the media are synchronous which is achieved
>> by setting FUA (Force Unit Access) bit in SCSI WRITE(10,12) commands.
>> This prevents I/O requests aggregation in block layer dramatically
>> decreasing performance.
>
>> diff --git a/drivers/usb/gadget/file_storage.c
>> b/drivers/usb/gadget/file_storage.c
>> index b49d86e..45f58d9 100644
>> --- a/drivers/usb/gadget/file_storage.c
>> +++ b/drivers/usb/gadget/file_storage.c
>> @@ -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?

>> @@ -743,6 +752,24 @@ static ssize_t fsg_store_ro(struct device *dev,
>> struct device_attribute *attr,
>>        return rc;
>>  }
>> +static ssize_t fsg_store_fua(struct device *dev, struct device_attribute
>> *attr,
>> +                            const char *buf, size_t count)
>> +{
>> +       ssize_t         rc = count;
>
> Not really needed here.
See below

>
>> +       struct fsg_lun  *curlun = fsg_lun_from_dev(dev);
>> +       int             i;
>> +
>> +       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.

>> +
>> +       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.
Actually fua = 1 means ignorance of that flag.


--
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Roger Quadros on 14 Jul 2010 10:00
On 07/14/2010 04:44 PM, ext Andy Shevchenko wrote:
> 2010/7/14 Michał Nazarewicz<m.nazarewicz(a)samsung.com>:
>> On Wed, 14 Jul 2010 11:05:31 +0200, Andy Shevchenko
>> <andy.shevchenko(a)gmail.com> wrote:
>>>
>>> MS Windows mounts removable storage in "Removal optimized mode" by
>>> default. All the writes to the media are synchronous which is achieved
>>> by setting FUA (Force Unit Access) bit in SCSI WRITE(10,12) commands.
>>> This prevents I/O requests aggregation in block layer dramatically
>>> decreasing performance.
>>
>>> diff --git a/drivers/usb/gadget/file_storage.c
>>> b/drivers/usb/gadget/file_storage.c
>>> index b49d86e..45f58d9 100644
>>> --- a/drivers/usb/gadget/file_storage.c
>>> +++ b/drivers/usb/gadget/file_storage.c
>>> @@ -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?
>
by partition do you mean medium? They are different terms.
A storage medium may have 1 or more logical partitions. It is left upto the
Host/user to decide how he wants to partition the medium.

file_storage driver does not deal with partitions. only mediums.

regards,
-roger
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Michał Nazarewicz on 14 Jul 2010 10:30
On Wed, 14 Jul 2010 15:44:58 +0200, Andy Shevchenko <andy.shevchenko(a)gmail.com> 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(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 |  Next  |  Last
Pages: 1 2 3
Prev: emacs and "linux" coding style
Next: [PATCH v2 2/2] ux500: add ab8500-regulators machine specific data