[PATCH] FAT: use blkdev_issue_flush() instead of congestion_wait()

[PATCH] FAT: use blkdev_issue_flush() instead of congestion_wait()

[NeilBrown]

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(-)

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;
 }
-- 
2.33.1

Re: [PATCH] FAT: use blkdev_issue_flush() instead of congestion_wait()

[Christoph Hellwig]

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>

Re: [PATCH] FAT: use blkdev_issue_flush() instead of congestion_wait()

[OGAWA Hirofumi]

"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;
>  }

-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH] FAT: use blkdev_issue_flush() instead of congestion_wait()

[NeilBrown]

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>
> 
> 

Re: [PATCH] FAT: use blkdev_issue_flush() instead of congestion_wait()

[OGAWA Hirofumi]

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

Re: [PATCH] FAT: use blkdev_issue_flush() instead of congestion_wait()

[NeilBrown]

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

[PATCH v2] FAT: use schedule_timeout_uninterruptible() instead of congestion_wait()

[NeilBrown]

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.  blkdev_issue_flush() is thought to be too expensive, so
replace congestion_wait() with an explicit timeout.

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..2321fb3eded5 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);
+		schedule_timeout_uninterruptible(HZ/10);
 	}
 	return 0;
 }
-- 
2.34.1

Re: [PATCH v2] FAT: use schedule_timeout_uninterruptible() instead of congestion_wait()

[OGAWA Hirofumi]

"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.  blkdev_issue_flush() is thought to be too expensive, so
> replace congestion_wait() with an explicit timeout.

If just replace, the following looks better

	set_current_state(TASK_UNINTERRUPTIBLE);
	io_schedule_timeout(HZ/10);

Otherwise,

Acked-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

> 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..2321fb3eded5 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);
> +		schedule_timeout_uninterruptible(HZ/10);
>  	}
>  	return 0;
>  }

-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH v2] FAT: use schedule_timeout_uninterruptible() instead of congestion_wait()

[NeilBrown]

On Sat, 11 Dec 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.  blkdev_issue_flush() is thought to be too expensive, so
> > replace congestion_wait() with an explicit timeout.
> 
> If just replace, the following looks better
> 
> 	set_current_state(TASK_UNINTERRUPTIBLE);
> 	io_schedule_timeout(HZ/10);
> 
> Otherwise,
> 
> Acked-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Thanks.
According to MAINTAINERS, I should send patches for this code to you,
with the implication (I assumed) that you would forwarded them upstream
if acceptable.
But the fact that you have send mt an Acked-By seems to suggest that you
won't be doing that.
To whom should I send this patch with your acked-by?

Thanks,
NeilBrown

Re: [PATCH v2] FAT: use schedule_timeout_uninterruptible() instead of congestion_wait()

[OGAWA Hirofumi]

"NeilBrown" <neilb@suse.de> writes:

> On Sat, 11 Dec 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.  blkdev_issue_flush() is thought to be too expensive, so
>> > replace congestion_wait() with an explicit timeout.
>> 
>> If just replace, the following looks better
>> 
>> 	set_current_state(TASK_UNINTERRUPTIBLE);
>> 	io_schedule_timeout(HZ/10);
>> 
>> Otherwise,
>> 
>> Acked-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
>
> Thanks.
> According to MAINTAINERS, I should send patches for this code to you,
> with the implication (I assumed) that you would forwarded them upstream
> if acceptable.
> But the fact that you have send mt an Acked-By seems to suggest that you
> won't be doing that.
> To whom should I send this patch with your acked-by?

Ah, sorry. I have no repository. So FAT patches goes to linus tree via
akpm's help.

So "Cc: Andrew Morton <akpm@linux-foundation.org>" and my Acked-by
should work (or I will Cc as reply if need).

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH v2] FAT: use schedule_timeout_uninterruptible() instead of congestion_wait()

[NeilBrown]

On Mon, 13 Dec 2021, OGAWA Hirofumi wrote:
> "NeilBrown" <neilb@suse.de> writes:
> 
> > On Sat, 11 Dec 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.  blkdev_issue_flush() is thought to be too expensive, so
> >> > replace congestion_wait() with an explicit timeout.
> >> 
> >> If just replace, the following looks better
> >> 
> >> 	set_current_state(TASK_UNINTERRUPTIBLE);
> >> 	io_schedule_timeout(HZ/10);
> >> 
> >> Otherwise,
> >> 
> >> Acked-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
> >
> > Thanks.
> > According to MAINTAINERS, I should send patches for this code to you,
> > with the implication (I assumed) that you would forwarded them upstream
> > if acceptable.
> > But the fact that you have send mt an Acked-By seems to suggest that you
> > won't be doing that.
> > To whom should I send this patch with your acked-by?
> 
> Ah, sorry. I have no repository. So FAT patches goes to linus tree via
> akpm's help.
> 
> So "Cc: Andrew Morton <akpm@linux-foundation.org>" and my Acked-by
> should work (or I will Cc as reply if need).

Will do, thanks.

NeilBrown

[PATCH v3] FAT: use io_schedule_timeout() instead of congestion_wait()

[NeilBrown]

congestion_wait() in this context is just a sleep - block devices do not
support congestion signalling any more.

The goal for this wait, which was introduced in Commit ae78bf9c4f5f
("[PATCH] add -o flush for fat") is to wait for any recently written
data to get to storage.  We currently have no direct mechanism to do
this, so a simple wait that behaves identically to the current
congestion_wait() is the best we can do.

This is a step towards removing congestion_wait()

Acked-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
Signed-off-by: NeilBrown <neilb@suse.de>
---

Hi Andrew,
 I believe you are an appropriate conduit for fs/fat patches once that
 have been suitably acked.
Thanks
NeilBrown

 fs/fat/file.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/fat/file.c b/fs/fat/file.c
index 13855ba49cd9..a5a309fcc7fa 100644
--- a/fs/fat/file.c
+++ b/fs/fat/file.c
@@ -175,9 +175,10 @@ 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);
+		set_current_state(TASK_UNINTERRUPTIBLE);
+		io_schedule_timeout(HZ/10);
 	}
 	return 0;
 }
-- 
2.34.1

Re: [PATCH] FAT: use blkdev_issue_flush() instead of congestion_wait()

[OGAWA Hirofumi]

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