Re: [PATCH v4] loop: don't print warnings if the underlying filesystem doesn't support discard

[Dave Chinner <david@fromorbit.com>]

On Wed, Oct 27, 2021 at 04:28:03AM -0400, Mikulas Patocka wrote:
> 
> 
> On Wed, 27 Oct 2021, Dave Chinner wrote:
> 
> > On Wed, Oct 13, 2021 at 05:28:36AM -0400, Mikulas Patocka wrote:
> > > Hi
> > > 
> > > Here I'm sending version 4 of the patch. It adds #include <linux/falloc.h> 
> > > to cifs and overlayfs to fix the bugs found out by the kernel test robot.
> > > 
> > > Mikulas
> > > 
> > > 
> > > 
> > > From: Mikulas Patocka <mpatocka@redhat.com>
> > > 
> > > The loop driver checks for the fallocate method and if it is present, it 
> > > assumes that the filesystem can do FALLOC_FL_ZERO_RANGE and 
> > > FALLOC_FL_PUNCH_HOLE requests. However, some filesystems (such as fat, or 
> > > tmpfs) have the fallocate method, but lack the capability to do 
> > > FALLOC_FL_ZERO_RANGE and/or FALLOC_FL_PUNCH_HOLE.
> > 
> > This seems like a loopback driver level problem, not something
> > filesystems need to solve. fallocate() is defined to return
> > -EOPNOTSUPP if a flag is passed that it does not support and that's
> > the mechanism used to inform callers that a fallocate function is
> > not supported by the underlying filesystem/storage.
> > 
> > Indeed, filesystems can support hole punching at the ->fallocate(),
> > but then return EOPNOTSUPP because certain dynamic conditions are
> > not met e.g. CIFS needs sparse file support on the server to support
> > hole punching, but we don't know this until we actually try to 
> > sparsify the file. IOWs, this patch doesn't address all the cases
> > where EOPNOTSUPP might actually get returned from filesystems and/or
> > storage.
> > 
> > > This results in syslog warnings "blk_update_request: operation not 
> > > supported error, dev loop0, sector 0 op 0x9:(WRITE_ZEROES) flags 0x800800 
> > > phys_seg 0 prio class 0". The error can be reproduced with this command: 
> > > "truncate -s 1GiB /tmp/file; losetup /dev/loop0 /tmp/file; blkdiscard -z 
> > > /dev/loop0"
> > 
> > Which I'm assuming comes from this:
> > 
> > 	        if (unlikely(error && !blk_rq_is_passthrough(req) &&
> >                      !(req->rq_flags & RQF_QUIET)))
> >                 print_req_error(req, error, __func__);
> > 
> > Which means we could supress the error message quite easily in
> > lo_fallocate() by doing:
> > 
> > out:
> > 	if (ret == -EOPNOTSUPP)
> > 		rq->rq_flags |= RQF_QUIET;
> > 	return ret;
> 
> I did this (see 
> https://lore.kernel.org/all/alpine.LRH.2.02.2109231539520.27863@file01.intranet.prod.int.rdu2.redhat.com/ 

Ok, you need to keep a changelog with the patch so that it's clear
what the history of it is....

> ) and Christoph Hellwig asked for a flag in the file_operations structure 
> ( https://lore.kernel.org/all/20210924155822.GA10064@lst.de/ ).

Looking at the code that has resulted, I think Christoph's
suggestion is a poor one. Code duplication is bad enough, worse is
that it's duplicating the open coding of non-trivial flag
combinations. Given that it is only needed for a single calling
context and it is unnecessary to solve the unique problem at hand
(suppress warning and turn off discard support) this makes it seem
like a case of over-engineering.

Further, it doesn't avoid the need for the loop device to handle
EOPNOTSUPP from fallocate directly, either, because as I explained
above "filesystem type supports the FALLOC_FL_PUNCH_HOLE API flag"
is not the same as "filesystem and/or file instance can execute
FALLOC_FL_PUNCH_HOLE"....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com