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

[Dave Chinner <david@fromorbit.com>]

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;

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@fromorbit.com