Re: [PATCH] Revert "block: Do not discard buffers under a mounted filesystem"

Re: [PATCH] Revert "block: Do not discard buffers under a mounted filesystem"

[Jan Kara]

On Tue 16-02-21 16:36:06, Christoph Hellwig wrote:
> On Tue, Feb 16, 2021 at 02:38:49PM +0100, Jan Kara wrote:
> > Apparently there are several userspace programs that depend on being
> > able to call BLKDISCARD ioctl without the ability to grab bdev
> > exclusively - namely FUSE filesystems have the device open without
> > O_EXCL (the kernel has the bdev open with O_EXCL) so the commit breaks
> > fstrim(8) for such filesystems. Also LVM when shrinking LV opens PV and
> > discards ranges released from LV but that PV may be already open
> > exclusively by someone else (see bugzilla link below for more details).
> > 
> > This reverts commit 384d87ef2c954fc58e6c5fd8253e4a1984f5fe02.
> 
> I think that is a bad idea. We fixed the problem for a reason.
> I think the right fix is to just do nothing if the device hasn't been
> opened with O_EXCL and can't be reopened with it, just don't do anything
> but also don't return an error.  After all discard and thus
> BLKDISCARD is purely advisory.

Yeah, certainly we'd have to fix the original problem in some other way.
Just silently ignoring BLKDISCARD if we cannot claim the device exclusively
is certainly an option to stop complaints from userspace. But note that
fstrim with fuse-based filesystem would still stay silent NOP which is
suboptimal. It could be fixed on FUSE side as I talked to Miklos but it
is not trivial. Similarly for the LVM regression...

I was wondering whether we could do something like:
	use truncate_inode_pages() if we can claim bdev exclusively
	use invalidate_inode_pages2_range() if we cannot claim bdev
          exclusively, possibly do nothing if that returns EBUSY?

The downside is that cases where we cannot claim bdev exclusively would
unnecessarily write dirty buffer cache before discard.

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] Revert "block: Do not discard buffers under a mounted filesystem"

[Jan Kara]

On Tue 16-02-21 18:16:09, Jan Kara wrote:
> On Tue 16-02-21 16:36:06, Christoph Hellwig wrote:
> > On Tue, Feb 16, 2021 at 02:38:49PM +0100, Jan Kara wrote:
> > > Apparently there are several userspace programs that depend on being
> > > able to call BLKDISCARD ioctl without the ability to grab bdev
> > > exclusively - namely FUSE filesystems have the device open without
> > > O_EXCL (the kernel has the bdev open with O_EXCL) so the commit breaks
> > > fstrim(8) for such filesystems. Also LVM when shrinking LV opens PV and
> > > discards ranges released from LV but that PV may be already open
> > > exclusively by someone else (see bugzilla link below for more details).
> > > 
> > > This reverts commit 384d87ef2c954fc58e6c5fd8253e4a1984f5fe02.
> > 
> > I think that is a bad idea. We fixed the problem for a reason.
> > I think the right fix is to just do nothing if the device hasn't been
> > opened with O_EXCL and can't be reopened with it, just don't do anything
> > but also don't return an error.  After all discard and thus
> > BLKDISCARD is purely advisory.
> 
> Yeah, certainly we'd have to fix the original problem in some other way.
> Just silently ignoring BLKDISCARD if we cannot claim the device exclusively
> is certainly an option to stop complaints from userspace. But note that
> fstrim with fuse-based filesystem would still stay silent NOP which is
> suboptimal. It could be fixed on FUSE side as I talked to Miklos but it
> is not trivial. Similarly for the LVM regression...
> 
> I was wondering whether we could do something like:
> 	use truncate_inode_pages() if we can claim bdev exclusively
> 	use invalidate_inode_pages2_range() if we cannot claim bdev
>           exclusively, possibly do nothing if that returns EBUSY?
> 
> The downside is that cases where we cannot claim bdev exclusively would
> unnecessarily write dirty buffer cache before discard.

OK, no more comments I guess so I'll post this in a form of a patch and
we'll see what people think.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR