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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 14 Jul 2010 14:38:20 +0200
From:	Michał Nazarewicz <m.nazarewicz@...sung.com>
To:	linux-kernel@...r.kernel.org,
	Andy Shevchenko <andy.shevchenko@...il.com>
Cc:	Andy Shevchenko <ext-andriy.shevchenko@...ia.com>,
	Denis Karpov <ext-denis.2.karpov@...ia.com>,
	Adrian Hunter <adrian.hunter@...ia.com>,
	Alan Stern <stern@...land.harvard.edu>,
	David Brownell <dbrownell@...rs.sourceforge.net>,
	Greg Kroah-Hartman <gregkh@...e.de>, linux-usb@...r.kernel.org
Subject: Re: [PATCHv2] usb: gadget: storage: optional SCSI WRITE FUA bit

On Wed, 14 Jul 2010 11:05:31 +0200, Andy Shevchenko <andy.shevchenko@...il.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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/