2019-06-28 14:27:20

by OGAWA Hirofumi

[permalink] [raw]
Subject: [PATCH] fat: Add nobarrier to workaround the strange behavior of device


There was the report of strange behavior of device with recent
blkdev_issue_flush() position change.

The following is simplified usbmon trace.

4203 9.160230 host -> 1.25.1 USBMS 95 SCSI: Synchronize Cache(10) LUN: 0x00 (LBA: 0x00000000, Len: 0)
4206 9.164911 1.25.1 -> host USBMS 77 SCSI: Response LUN: 0x00 (Synchronize Cache(10)) (Good)
4207 9.323927 host -> 1.25.1 USBMS 95 SCSI: Read(10) LUN: 0x00 (LBA: 0x00279950, Len: 240)
4212 9.327138 1.25.1 -> host USBMS 77 SCSI: Response LUN: 0x00 (Read(10)) (Good)

[...]

7323 10.202167 host -> 1.25.1 USBMS 95 SCSI: Synchronize Cache(10) LUN: 0x00 (LBA: 0x00000000, Len: 0)
7326 10.432266 1.25.1 -> host USBMS 77 SCSI: Response LUN: 0x00 (Synchronize Cache(10)) (Good)
7327 10.769092 host -> 1.25.1 USBMS 95 SCSI: Test Unit Ready LUN: 0x00
7330 10.769192 1.25.1 -> host USBMS 77 SCSI: Response LUN: 0x00 (Test Unit Ready) (Good)
7335 12.849093 host -> 1.25.1 USBMS 95 SCSI: Test Unit Ready LUN: 0x00
7338 12.849206 1.25.1 -> host USBMS 77 SCSI: Response LUN: 0x00 (Test Unit Ready) (Check Condition)
7339 12.849209 host -> 1.25.1 USBMS 95 SCSI: Request Sense LUN: 0x00

If "Synchronize Cache" command issued then there is idle time, the
device stop to process further commands, and behave as like no media.
(it returns NOT_READY [MEDIUM NOT PRESENT] for SENSE command, and this
happened on Kindle) [just a guess, the device is trying to detect the
"safe-unplug" operation of Windows or such?]

To workaround those devices and provide flexibility, this adds
"barrier"/"nobarrier" mount options to fat driver.

Cc: <[email protected]>
Signed-off-by: OGAWA Hirofumi <[email protected]>
---

fs/fat/fat.h | 1 +
fs/fat/file.c | 8 ++++++--
fs/fat/inode.c | 16 ++++++++++++++--
3 files changed, 21 insertions(+), 4 deletions(-)

diff -puN fs/fat/fat.h~fat-nobarrier fs/fat/fat.h
--- linux/fs/fat/fat.h~fat-nobarrier 2019-06-28 21:22:18.146191739 +0900
+++ linux-hirofumi/fs/fat/fat.h 2019-06-28 21:59:11.693934616 +0900
@@ -51,6 +51,7 @@ struct fat_mount_options {
tz_set:1, /* Filesystem timestamps' offset set */
rodir:1, /* allow ATTR_RO for directory */
discard:1, /* Issue discard requests on deletions */
+ barrier:1, /* Issue FLUSH command */
dos1xfloppy:1; /* Assume default BPB for DOS 1.x floppies */
};

diff -puN fs/fat/file.c~fat-nobarrier fs/fat/file.c
--- linux/fs/fat/file.c~fat-nobarrier 2019-06-28 21:22:18.147191734 +0900
+++ linux-hirofumi/fs/fat/file.c 2019-06-28 21:59:11.693934616 +0900
@@ -193,17 +193,21 @@ static int fat_file_release(struct inode
int fat_file_fsync(struct file *filp, loff_t start, loff_t end, int datasync)
{
struct inode *inode = filp->f_mapping->host;
+ struct msdos_sb_info *sbi = MSDOS_SB(inode->i_sb);
int err;

err = __generic_file_fsync(filp, start, end, datasync);
if (err)
return err;

- err = sync_mapping_buffers(MSDOS_SB(inode->i_sb)->fat_inode->i_mapping);
+ err = sync_mapping_buffers(sbi->fat_inode->i_mapping);
if (err)
return err;

- return blkdev_issue_flush(inode->i_sb->s_bdev, GFP_KERNEL, NULL);
+ if (sbi->options.barrier)
+ err = blkdev_issue_flush(inode->i_sb->s_bdev, GFP_KERNEL, NULL);
+
+ return err;
}


diff -puN fs/fat/inode.c~fat-nobarrier fs/fat/inode.c
--- linux/fs/fat/inode.c~fat-nobarrier 2019-06-28 21:22:18.148191730 +0900
+++ linux-hirofumi/fs/fat/inode.c 2019-06-28 21:59:11.694934611 +0900
@@ -1016,6 +1016,8 @@ static int fat_show_options(struct seq_f
seq_puts(m, ",nfs=stale_rw");
if (opts->discard)
seq_puts(m, ",discard");
+ if (!opts->barrier)
+ seq_puts(m, ",nobarrier");
if (opts->dos1xfloppy)
seq_puts(m, ",dos1xfloppy");

@@ -1031,8 +1033,9 @@ enum {
Opt_shortname_winnt, Opt_shortname_mixed, Opt_utf8_no, Opt_utf8_yes,
Opt_uni_xl_no, Opt_uni_xl_yes, Opt_nonumtail_no, Opt_nonumtail_yes,
Opt_obsolete, Opt_flush, Opt_tz_utc, Opt_rodir, Opt_err_cont,
- Opt_err_panic, Opt_err_ro, Opt_discard, Opt_nfs, Opt_time_offset,
- Opt_nfs_stale_rw, Opt_nfs_nostale_ro, Opt_err, Opt_dos1xfloppy,
+ Opt_err_panic, Opt_err_ro, Opt_discard, Opt_barrier, Opt_nobarrier,
+ Opt_nfs, Opt_time_offset, Opt_nfs_stale_rw, Opt_nfs_nostale_ro,
+ Opt_err, Opt_dos1xfloppy,
};

static const match_table_t fat_tokens = {
@@ -1062,6 +1065,8 @@ static const match_table_t fat_tokens =
{Opt_err_panic, "errors=panic"},
{Opt_err_ro, "errors=remount-ro"},
{Opt_discard, "discard"},
+ {Opt_barrier, "barrier"},
+ {Opt_nobarrier, "nobarrier"},
{Opt_nfs_stale_rw, "nfs"},
{Opt_nfs_stale_rw, "nfs=stale_rw"},
{Opt_nfs_nostale_ro, "nfs=nostale_ro"},
@@ -1146,6 +1151,7 @@ static int parse_options(struct super_bl
opts->numtail = 1;
opts->usefree = opts->nocase = 0;
opts->tz_set = 0;
+ opts->barrier = 1;
opts->nfs = 0;
opts->errors = FAT_ERRORS_RO;
*debug = 0;
@@ -1335,6 +1341,12 @@ static int parse_options(struct super_bl
case Opt_discard:
opts->discard = 1;
break;
+ case Opt_barrier:
+ opts->barrier = 1;
+ break;
+ case Opt_nobarrier:
+ opts->barrier = 0;
+ break;

/* obsolete mount options */
case Opt_obsolete:
_

--
OGAWA Hirofumi <[email protected]>


2019-06-28 14:33:45

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [PATCH] fat: Add nobarrier to workaround the strange behavior of device

On Fri, Jun 28, 2019 at 11:18:19PM +0900, OGAWA Hirofumi wrote:
> To workaround those devices and provide flexibility, this adds
> "barrier"/"nobarrier" mount options to fat driver.

We have deprecated these rather misnamed options, and now instead allow
tweaking the 'cache_type' attribute on the SCSI device.

That being said if the device behave this buggy you should also report
it to to the usb-storage and scsi maintainers so that we can add a
quirk for it.

2019-06-28 15:04:35

by OGAWA Hirofumi

[permalink] [raw]
Subject: Re: [PATCH] fat: Add nobarrier to workaround the strange behavior of device

Christoph Hellwig <[email protected]> writes:

> On Fri, Jun 28, 2019 at 11:18:19PM +0900, OGAWA Hirofumi wrote:
>> To workaround those devices and provide flexibility, this adds
>> "barrier"/"nobarrier" mount options to fat driver.
>
> We have deprecated these rather misnamed options, and now instead allow
> tweaking the 'cache_type' attribute on the SCSI device.

I see, sounds like good though. Does it work for all stable versions?
Can it disable only flush command without other effect? And it would be
better to be normal user controllable easily.

This happened on normal user's calibre app that mount via udisks. With
this option, user can workaround with /etc/fstab for immediate users.

> That being said if the device behave this buggy you should also report
> it to to the usb-storage and scsi maintainers so that we can add a
> quirk for it.

It might not be able to say as buggy simply. The device looks work if no
idle and not hit pattern of usage, so quirk can be overkill.

Anyway, I don't have the device, if you can get the device and
investigate, it can be fine.

Thanks.
--
OGAWA Hirofumi <[email protected]>

2019-06-28 16:03:12

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [PATCH] fat: Add nobarrier to workaround the strange behavior of device

On Sat, Jun 29, 2019 at 12:03:46AM +0900, OGAWA Hirofumi wrote:
> I see, sounds like good though. Does it work for all stable versions?
> Can it disable only flush command without other effect? And it would be
> better to be normal user controllable easily.

The option was added in 2.6.17, so it's been around forever. But
no, it obviously is not user exposed as using it on a normal drive
can lead to data loss.

2019-06-28 16:28:25

by OGAWA Hirofumi

[permalink] [raw]
Subject: Re: [PATCH] fat: Add nobarrier to workaround the strange behavior of device

Christoph Hellwig <[email protected]> writes:

> On Sat, Jun 29, 2019 at 12:03:46AM +0900, OGAWA Hirofumi wrote:
>> I see, sounds like good though. Does it work for all stable versions?
>> Can it disable only flush command without other effect? And it would be
>> better to be normal user controllable easily.
>
> The option was added in 2.6.17, so it's been around forever. But
> no, it obviously is not user exposed as using it on a normal drive
> can lead to data loss.

I see. It sounds like good as long term solution though (if no effect
other than disabling flush command), it may need some monitor daemon and
detect the device, and apply user policy as root. (BTW, I meant,
workaround is normal user controllable with config by root or such)

I think, it may not be good as short term solution for especially stable
users. If there is no better short term solution, I would like to still
push this patch as short term workaround.

Thanks.
--
OGAWA Hirofumi <[email protected]>