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

[Lukas Czerner <lczerner@redhat.com>]

On Mon, Jan 03, 2022 at 04:34:40PM +0100, Martin Wilck wrote:
> 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.

Not really, the sentence you're pointing out above was there from the
beginning. Commit ce3d198d7 didn't change that.

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

This makes absolutely no difference from the user POV. Enough nitpicking,
feel free to cc me if you decide to send a patch.

Thanks!
-Lukas