| Message ID | 163712349419.13692.2859038330142282757@noble.neil.brown.name (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | FAT: use blkdev_issue_flush() instead of congestion_wait() | expand |
On Wed, Nov 17, 2021 at 03:31:34PM +1100, NeilBrown wrote: > > congestion_wait() in this context is just a sleep - block devices do not > in general support congestion signalling any more. > > The goal here is to wait for any recently written data to get to > storage. This can be achieved using blkdev_issue_flush(). This goes back to commit ae78bf9c4f5fde3c67e2829505f195d7347ce3e4 and looking at that this seems to be correct. It might be worth to reference that commit, though. Otherwise looks good: Reviewed-by: Christoph Hellwig <hch@lst.de>
"NeilBrown" <neilb@suse.de> writes: > congestion_wait() in this context is just a sleep - block devices do not > in general support congestion signalling any more. > > The goal here is to wait for any recently written data to get to > storage. This can be achieved using blkdev_issue_flush(). Purpose of flush option should be for making umount faster, not data integrity. (but current flush implement is strange at several places, IMO) So, I don't think the issue_flush is not proper for it (flush is very slow on some usb thumb), and rather I think it is better off to just remove the congestion_wait(). Thanks. > Signed-off-by: NeilBrown <neilb@suse.de> > --- > fs/fat/file.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/fs/fat/file.c b/fs/fat/file.c > index 13855ba49cd9..c50a52f40e37 100644 > --- a/fs/fat/file.c > +++ b/fs/fat/file.c > @@ -175,9 +175,9 @@ long fat_generic_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) > static int fat_file_release(struct inode *inode, struct file *filp) > { > if ((filp->f_mode & FMODE_WRITE) && > - MSDOS_SB(inode->i_sb)->options.flush) { > + MSDOS_SB(inode->i_sb)->options.flush) { > fat_flush_inodes(inode->i_sb, inode, NULL); > - congestion_wait(BLK_RW_ASYNC, HZ/10); > + blkdev_issue_flush(inode->i_sb->s_bdev); > } > return 0; > }
On Sun, 21 Nov 2021, OGAWA Hirofumi wrote: > "NeilBrown" <neilb@suse.de> writes: > > > congestion_wait() in this context is just a sleep - block devices do not > > in general support congestion signalling any more. > > > > The goal here is to wait for any recently written data to get to > > storage. This can be achieved using blkdev_issue_flush(). > > Purpose of flush option should be for making umount faster, not data > integrity. (but current flush implement is strange at several places, IMO) I don't think that is true. I believe the purpose of the flush option is to write out data as soon as a file is closed, so that if the media is removed without first unmounting, the data is more likely to be safe. That is why the commit which introduce it: Commit ae78bf9c4f5f ("[PATCH] add -o flush for fat") particularly mentions "removable media". > > So, I don't think the issue_flush is not proper for it (flush is very > slow on some usb thumb), and rather I think it is better off to just > remove the congestion_wait(). We already call blkdev_issue_flush() on fsync. With my patch, a simple close() effective becomes an fsync() and a close(). I think that is completely consistent with the purpose of "-o flush". Thanks, NeilBrown > > Thanks. > > > Signed-off-by: NeilBrown <neilb@suse.de> > > --- > > fs/fat/file.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/fs/fat/file.c b/fs/fat/file.c > > index 13855ba49cd9..c50a52f40e37 100644 > > --- a/fs/fat/file.c > > +++ b/fs/fat/file.c > > @@ -175,9 +175,9 @@ long fat_generic_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) > > static int fat_file_release(struct inode *inode, struct file *filp) > > { > > if ((filp->f_mode & FMODE_WRITE) && > > - MSDOS_SB(inode->i_sb)->options.flush) { > > + MSDOS_SB(inode->i_sb)->options.flush) { > > fat_flush_inodes(inode->i_sb, inode, NULL); > > - congestion_wait(BLK_RW_ASYNC, HZ/10); > > + blkdev_issue_flush(inode->i_sb->s_bdev); > > } > > return 0; > > } > > -- > OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> > >
"NeilBrown" <neilb@suse.de> writes: > On Sun, 21 Nov 2021, OGAWA Hirofumi wrote: >> "NeilBrown" <neilb@suse.de> writes: >> >> > congestion_wait() in this context is just a sleep - block devices do not >> > in general support congestion signalling any more. >> > >> > The goal here is to wait for any recently written data to get to >> > storage. This can be achieved using blkdev_issue_flush(). >> >> Purpose of flush option should be for making umount faster, not data >> integrity. (but current flush implement is strange at several places, IMO) > > I don't think that is true. I believe the purpose of the flush option > is to write out data as soon as a file is closed, so that if the media > is removed without first unmounting, the data is more likely to be safe. > That is why the commit which introduce it: > Commit ae78bf9c4f5f ("[PATCH] add -o flush for fat") > particularly mentions "removable media". Right. This was to make the removable device usage better (but sync option is too slow). e.g. # cp -a /foo/source /mnt/fatfs # umount <don't too slow> or <do other thing, and forget umount> >> So, I don't think the issue_flush is not proper for it (flush is very >> slow on some usb thumb), and rather I think it is better off to just >> remove the congestion_wait(). > > We already call blkdev_issue_flush() on fsync. With my patch, a simple > close() effective becomes an fsync() and a close(). I think that is > completely consistent with the purpose of "-o flush". It makes much slower above "cp -a" part. So I think it is overkill.
On Mon, 22 Nov 2021, OGAWA Hirofumi wrote: > "NeilBrown" <neilb@suse.de> writes: > > > On Sun, 21 Nov 2021, OGAWA Hirofumi wrote: > >> "NeilBrown" <neilb@suse.de> writes: > >> > >> > congestion_wait() in this context is just a sleep - block devices do not > >> > in general support congestion signalling any more. > >> > > >> > The goal here is to wait for any recently written data to get to > >> > storage. This can be achieved using blkdev_issue_flush(). > >> > >> Purpose of flush option should be for making umount faster, not data > >> integrity. (but current flush implement is strange at several places, IMO) > > > > I don't think that is true. I believe the purpose of the flush option > > is to write out data as soon as a file is closed, so that if the media > > is removed without first unmounting, the data is more likely to be safe. > > That is why the commit which introduce it: > > Commit ae78bf9c4f5f ("[PATCH] add -o flush for fat") > > particularly mentions "removable media". > > Right. This was to make the removable device usage better (but sync > option is too slow). > > e.g. > # cp -a /foo/source /mnt/fatfs > > # umount <don't too slow> > or > <do other thing, and forget umount> or use GUI to drag a file to the removable device, wait for the copy to appear to finish, then pull the device. sync is too slow as it flush each change to storage as it happens. Each block, each FA-Table update etc. "-o flush" does the flush as file-close rather than on each write. But it still needs to provide the same safety. i.e. write and flush and wait. > > >> So, I don't think the issue_flush is not proper for it (flush is very > >> slow on some usb thumb), and rather I think it is better off to just > >> remove the congestion_wait(). > > > > We already call blkdev_issue_flush() on fsync. With my patch, a simple > > close() effective becomes an fsync() and a close(). I think that is > > completely consistent with the purpose of "-o flush". > > It makes much slower above "cp -a" part. So I think it is overkill. There doesn't seem to be any point to "-o flush" if it doesn't promise anything. Without the blkdeV_issue_flush() we have no guarantee that the data is safe after the file is closed - do we? Certainly it will be slower than without "-o flush", but that is the price you pay for safety. However, if you are adamant then let's just put in a timeout. Patch to follow. Thanks, NeilBrown
"NeilBrown" <neilb@suse.de> writes: > On Mon, 22 Nov 2021, OGAWA Hirofumi wrote: >> "NeilBrown" <neilb@suse.de> writes: >> >> Right. This was to make the removable device usage better (but sync >> option is too slow). >> >> e.g. >> # cp -a /foo/source /mnt/fatfs >> >> # umount <don't too slow> >> or >> <do other thing, and forget umount> > > or use GUI to drag a file to the removable device, wait for the copy to > appear to finish, then pull the device. > > sync is too slow as it flush each change to storage as it happens. Each > block, each FA-Table update etc. > > "-o flush" does the flush as file-close rather than on each write. > But it still needs to provide the same safety. i.e. write and flush and > wait. If you want to provide the data integrity, e.g., you have to check and sync parent recursively. And more. >> It makes much slower above "cp -a" part. So I think it is overkill. > > There doesn't seem to be any point to "-o flush" if it doesn't promise > anything. Without the blkdeV_issue_flush() we have no guarantee that > the data is safe after the file is closed - do we? > Certainly it will be slower than without "-o flush", but that is the > price you pay for safety. What I originally think to provide here (before this patch) is, removable media capable flusher (early flush) + some sort of auto unmount (syncfs when fs becomes idle.). IOW, nearly maximum throughput and better safety than default fluser. But never guarantee anything the data integrity, otherwise this option becomes too complex and heavyweight. Thanks.
diff --git a/fs/fat/file.c b/fs/fat/file.c index 13855ba49cd9..c50a52f40e37 100644 --- a/fs/fat/file.c +++ b/fs/fat/file.c @@ -175,9 +175,9 @@ long fat_generic_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) static int fat_file_release(struct inode *inode, struct file *filp) { if ((filp->f_mode & FMODE_WRITE) && - MSDOS_SB(inode->i_sb)->options.flush) { + MSDOS_SB(inode->i_sb)->options.flush) { fat_flush_inodes(inode->i_sb, inode, NULL); - congestion_wait(BLK_RW_ASYNC, HZ/10); + blkdev_issue_flush(inode->i_sb->s_bdev); } return 0; }
congestion_wait() in this context is just a sleep - block devices do not in general support congestion signalling any more. The goal here is to wait for any recently written data to get to storage. This can be achieved using blkdev_issue_flush(). Signed-off-by: NeilBrown <neilb@suse.de> --- fs/fat/file.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)