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

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

[OGAWA Hirofumi]

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: <stable@vger.kernel.org>
Signed-off-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
---

 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 <hirofumi@mail.parknet.co.jp>

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

[Christoph Hellwig]

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.

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

[OGAWA Hirofumi]

Christoph Hellwig <hch@infradead.org> 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 <hirofumi@mail.parknet.co.jp>

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

[Christoph Hellwig]

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.

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

[OGAWA Hirofumi]

Christoph Hellwig <hch@infradead.org> 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 <hirofumi@mail.parknet.co.jp>