linux-kernel - Re: [PATCH] FAT: use blkdev_issue_flush() instead of congestion

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-id: <163911011670.9928.6438117555175130033@noble.neil.brown.name>
Date:   Fri, 10 Dec 2021 15:21:56 +1100
From:   "NeilBrown" <neilb@...e.de>
To:     "OGAWA Hirofumi" <hirofumi@...l.parknet.co.jp>
Cc:     linux-kernel@...r.kernel.org, linux-block@...r.kernel.org,
        Christoph Hellwig <hch@...radead.org>
Subject: Re: [PATCH] FAT: use blkdev_issue_flush() instead of congestion_wait()

On Mon, 22 Nov 2021, OGAWA Hirofumi wrote:
> "NeilBrown" <neilb@...e.de> writes:
> 
> > On Sun, 21 Nov 2021, OGAWA Hirofumi wrote:
> >> "NeilBrown" <neilb@...e.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