Re: [BUG REPORT] General protection fault while discarding extents on XFS on next-20240305

[Christoph Hellwig <hch@infradead.org>]

On Thu, Mar 07, 2024 at 07:51:35AM +1100, Dave Chinner wrote:
> On Wed, Mar 06, 2024 at 07:18:02AM -0800, Christoph Hellwig wrote:
> > Lookings at this a bit more I'm not sure my fix is enough as the error
> > handling is really complex.  Also given that some discard callers are
> > from kernel threads messing with interruptibility I'm not entirely
> > sure that having this check in the common helper is a good idea.
> 
> Yeah, this seems like a problem. The only places that userspace
> should be issuing discards directly and hence be interruptible from
> are FITRIM, BLKDISCARD and fallocate() on block devices.

Yes.

> Filesystems already handle fatal signals in FITRIM (e.g. see
> xfs_trim_should_stop(), ext4_trim_interrupted(),
> btrfs_trim_free_extents(), etc), so it seems to me that the only
> non-interruptible call from userspace are operations directly on
> block devices which have no higher level iteration over the range to
> discard and the user controls the range directly.

Yeah.

> Perhaps the solution is to change BLKDISCARD/fallocate() on bdev to
> look more like xfs_discard_extents() where it breaks the range up
> into smaller chunks and intersperses bio chaining with signal
> checks.

Well, xfs_discard_extents has different extents from the higher
layers.  __blkdev_issue_discard than breaks it up based on what
fits into the bio (and does some alignment against our normal
rule of leaving that to the splitting code).  But I suspect moving
the loop in __blkdev_issue_discard into the callers could really
help with this.

> 
> I suspect the same solution is necessary for blkdev_issue_zeroout()
> and blkdev_issue_secure_erase(), because both of them have user
> controlled lengths...

Yes.  (or rather two sub cases of the former and the latter)