| Message ID | 871rzdrdxw.fsf@mail.parknet.co.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fat: Add nobarrier to workaround the strange behavior of device | expand |
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.
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.
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.
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.
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:
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(-)