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

[Mikulas Patocka]

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 at 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/ 
) and Christoph Hellwig asked for a flag in the file_operations structure 
( https://lore.kernel.org/all/20210924155822.GA10064@lst.de/ ).

Mikulas

> And then we can also run blk_queue_flag_clear(QUEUE_FLAG_DISCARD)
> (and whatever else is needed to kill discards) to turn off future
> discard attempts on that loopback device. This way the problem is
> just quietly and correctly handled by the loop device and everything
> is good...
> 
> Thoughts?
> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david at fromorbit.com
>