Re: [PATCH] ext4: Avoid trim error on fs with small groups

[Martin Wilck <mwilck@suse.com>]

On Mon, 2021-11-22 at 14:53 +0100, Lukas Czerner wrote:
> On Fri, Nov 19, 2021 at 04:43:53PM +0100, Martin Wilck wrote:
> > Hi Martin, Lukas,
> > 
> > On Tue, 2021-11-16 at 23:35 -0500, Martin K. Petersen wrote:
> > 
> 
> Thanks you Martin P. for clarifying.
> 
> > 
> > I've checked btrfs and xfs, and they treat the minlen like Jan's
> > patch
> > would - the minlen is set to the device's granularity, and
> > discarding
> > smaller ranges is silently not even attempted.
> > 
> > So this seems to be common practice among file system
> > implementations,
> > and thus Jan's patch would make ext4 behave like the others. But
> > I'm
> > still uncertain if this behavior is ideal, and your remarks seem to
> > confirm that.
> 
> Yeah I modeled my change for ext4 on the work in xfs done by
> Christoph
> and so it kind of spread organically to other file systems.

Ok. I still believe that it's not ideal this way, but this logic is
only applied in corner cases AFAICS, so it doesn't really hurt.

I'm fine with Jan's patch for the time being.

> 
> > 
> > The fstrim man page text is highly misleading. "The default value
> > is
> > zero, discarding every free block" is obviously wrong, given the
> > current actual behavior of the major filesystems.
> 
> Originally there was no discard granularity optimization so it is
> what
> it meant. 

Not quite. That man page text is from 2019, commit ce3d198d7 ("fstrim:
document kernel return minlen explicitly"). At that time,
discard_granularity had existed for 10 years already.


> And it also says "fstrim will adjust the minimum if it's
> smaller than the device's minimum". So I am not sure if it's
> necessarily
> misleading.

It is misleading, because it's not fstrim that adjusts anything, but
the file system code in the kernel.

> 
> Still I think it should be best effort from the fs, not guarantee.
> 
> > 
> > The way this is currently implemented, the user has no way to
> > actually
> > "discard every free block" to the extent supported by the device. I
> > think that being able to do this would be desirable, at least in
> > certain situations.
> > 
> > If we changed this, with the default value of 0 used by fstrim,
> > file
> > systems would attempt to free every block, which is slow and would
> > likely either fail or have no effect on may devices. Maybe we
> > should
> > treat the value "0" like "automatic", i.e. adjust it the minlen to
> > the
> > device's granularity like we do now, but leave the value unchanged
> > if
> > the user explicitly sets a small non-zero value? That way "fstrim -
> > m
> > 512" could be used to force discarding every block that can be
> > discarded.
> 
> Perhaps, this sounds like a reasonable solution to me.

This could be implemented in a second step, then.

Thanks,
Martin