From: Dave Chinner <david@fromorbit.com> To: Mikulas Patocka <mpatocka@redhat.com> Cc: Christoph Hellwig <hch@lst.de>, Jens Axboe <axboe@kernel.dk>, Ming Lei <ming.lei@redhat.com>, Zdenek Kabelac <zkabelac@redhat.com>, linux-block@vger.kernel.org, dm-devel@redhat.com, linux-fsdevel@vger.kernel.org Subject: Re: [PATCH v4] loop: don't print warnings if the underlying filesystem doesn't support discard Date: Thu, 28 Oct 2021 15:15:13 +1100 [thread overview] Message-ID: <20211028041513.GD4821@dread.disaster.area> (raw) In-Reply-To: <alpine.LRH.2.02.2110270421380.10452@file01.intranet.prod.int.rdu2.redhat.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
WARNING: multiple messages have this Message-ID (diff)
From: Dave Chinner <david@fromorbit.com> To: Mikulas Patocka <mpatocka@redhat.com> Cc: Jens Axboe <axboe@kernel.dk>, Ming Lei <ming.lei@redhat.com>, linux-block@vger.kernel.org, dm-devel@redhat.com, Zdenek Kabelac <zkabelac@redhat.com>, linux-fsdevel@vger.kernel.org, Christoph Hellwig <hch@lst.de> Subject: Re: [dm-devel] [PATCH v4] loop: don't print warnings if the underlying filesystem doesn't support discard Date: Thu, 28 Oct 2021 15:15:13 +1100 [thread overview] Message-ID: <20211028041513.GD4821@dread.disaster.area> (raw) In-Reply-To: <alpine.LRH.2.02.2110270421380.10452@file01.intranet.prod.int.rdu2.redhat.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 -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
next prev parent reply other threads:[~2021-10-28 4:15 UTC|newest] Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-09-23 19:48 [PATCH] loop: don't print warnings if the underlying filesystem doesn't support discard Mikulas Patocka 2021-09-24 0:17 ` Ming Lei 2021-09-24 15:58 ` Christoph Hellwig 2021-10-04 13:01 ` [dm-devel] " Mikulas Patocka 2021-10-04 13:01 ` Mikulas Patocka 2021-10-12 6:20 ` Christoph Hellwig 2021-10-12 6:20 ` [dm-devel] " Christoph Hellwig 2021-10-12 20:25 ` [PATCH v3] " Mikulas Patocka 2021-10-12 20:25 ` [dm-devel] " Mikulas Patocka 2021-10-13 2:56 ` kernel test robot 2021-10-13 2:56 ` kernel test robot 2021-10-13 2:56 ` [dm-devel] " kernel test robot 2021-10-13 5:06 ` kernel test robot 2021-10-13 5:06 ` kernel test robot 2021-10-13 5:06 ` [dm-devel] " kernel test robot 2021-10-13 9:28 ` [PATCH v4] " Mikulas Patocka 2021-10-13 9:28 ` [dm-devel] " Mikulas Patocka 2021-10-27 5:02 ` Dave Chinner 2021-10-27 5:02 ` [dm-devel] " Dave Chinner 2021-10-27 8:28 ` Mikulas Patocka 2021-10-27 8:28 ` [dm-devel] " Mikulas Patocka 2021-10-28 4:15 ` Dave Chinner [this message] 2021-10-28 4:15 ` Dave Chinner
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20211028041513.GD4821@dread.disaster.area \ --to=david@fromorbit.com \ --cc=axboe@kernel.dk \ --cc=dm-devel@redhat.com \ --cc=hch@lst.de \ --cc=linux-block@vger.kernel.org \ --cc=linux-fsdevel@vger.kernel.org \ --cc=ming.lei@redhat.com \ --cc=mpatocka@redhat.com \ --cc=zkabelac@redhat.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.