From: Dave Chinner <firstname.lastname@example.org> To: "Darrick J. Wong" <email@example.com> Cc: Ian Lance Taylor <firstname.lastname@example.org>, Greg KH <email@example.com>, Nicolas Boichat <firstname.lastname@example.org>, Alexander Viro <email@example.com>, Luis Lozano <firstname.lastname@example.org>, email@example.com, lkml <firstname.lastname@example.org> Subject: Re: [PATCH 1/6] fs: Add flag to file_system_type to indicate content is generated Date: Mon, 15 Feb 2021 11:38:55 +1100 [thread overview] Message-ID: <20210215003855.GY4626@dread.disaster.area> (raw) In-Reply-To: <20210212235448.GH7187@magnolia> On Fri, Feb 12, 2021 at 03:54:48PM -0800, Darrick J. Wong wrote: > On Sat, Feb 13, 2021 at 10:27:26AM +1100, Dave Chinner wrote: > > On Fri, Feb 12, 2021 at 03:07:39PM -0800, Ian Lance Taylor wrote: > > > On Fri, Feb 12, 2021 at 3:03 PM Dave Chinner <email@example.com> wrote: > > > > > > > > On Fri, Feb 12, 2021 at 04:45:41PM +0100, Greg KH wrote: > > > > > On Fri, Feb 12, 2021 at 07:33:57AM -0800, Ian Lance Taylor wrote: > > > > > > On Fri, Feb 12, 2021 at 12:38 AM Greg KH <firstname.lastname@example.org> wrote: > > > > > > > > > > > > > > Why are people trying to use copy_file_range on simple /proc and /sys > > > > > > > files in the first place? They can not seek (well most can not), so > > > > > > > that feels like a "oh look, a new syscall, let's use it everywhere!" > > > > > > > problem that userspace should not do. > > > > > > > > > > > > This may have been covered elsewhere, but it's not that people are > > > > > > saying "let's use copy_file_range on files in /proc." It's that the > > > > > > Go language standard library provides an interface to operating system > > > > > > files. When Go code uses the standard library function io.Copy to > > > > > > copy the contents of one open file to another open file, then on Linux > > > > > > kernels 5.3 and greater the Go standard library will use the > > > > > > copy_file_range system call. That seems to be exactly what > > > > > > copy_file_range is intended for. Unfortunately it appears that when > > > > > > people writing Go code open a file in /proc and use io.Copy the > > > > > > contents to another open file, copy_file_range does nothing and > > > > > > reports success. There isn't anything on the copy_file_range man page > > > > > > explaining this limitation, and there isn't any documented way to know > > > > > > that the Go standard library should not use copy_file_range on certain > > > > > > files. > > > > > > > > > > But, is this a bug in the kernel in that the syscall being made is not > > > > > working properly, or a bug in that Go decided to do this for all types > > > > > of files not knowing that some types of files can not handle this? > > > > > > > > > > If the kernel has always worked this way, I would say that Go is doing > > > > > the wrong thing here. If the kernel used to work properly, and then > > > > > changed, then it's a regression on the kernel side. > > > > > > > > > > So which is it? > > > > > > > > Both Al Viro and myself have said "copy file range is not a generic > > > > method for copying data between two file descriptors". It is a > > > > targetted solution for *regular files only* on filesystems that store > > > > persistent data and can accelerate the data copy in some way (e.g. > > > > clone, server side offload, hardware offlead, etc). It is not > > > > intended as a copy mechanism for copying data from one random file > > > > descriptor to another. > > > > > > > > The use of it as a general file copy mechanism in the Go system > > > > library is incorrect and wrong. It is a userspace bug. Userspace > > > > has done the wrong thing, userspace needs to be fixed. > > > > > > OK, we'll take it out. > > > > > > I'll just make one last plea that I think that copy_file_range could > > > be much more useful if there were some way that a program could know > > > whether it would work or not. > > Well... we could always implement a CFR_DRYRUN flag that would run > through all the parameter validation and return 0 just before actually > starting any real copying logic. But that wouldn't itself solve the > problem that there are very old virtual filesystems in Linux that have > zero-length regular files that behave like a pipe. > > > If you can't tell from userspace that a file has data in it other > > than by calling read() on it, then you can't use cfr on it. > > I don't know how to do that, Dave. :) If stat returns a non-zero size, then userspace knows it has at least that much data in it, whether it be zeros or previously written data. cfr will copy that data. The special zero length regular pipe files fail this simple "how much data is there to copy in this file" check... > Frankly I'm with the Go developers on this -- one should detect c_f_r by > calling it and if it errors out then fall back to the usual userspace > buffer copy strategy. > > That still means we need to fix the kernel WRT these weird old > filesystems. One of... And that is the whole problem here, not that cfr is failing. cfr is behaving correctly and consistently as the filesystem is telling the kernel there is no data in the file (i.e. size = 0). > 1. Get rid of the generic fallback completely, since splice only copies > 64k at a time and ... yay? I guess it at least passes generic/521 and > generic/522 these days. I've had a few people ask me for cfr to not fall back to a manual copy because they only want it to do something if it can accelerate the copy to be faster than userspace can copy the data itself. If the filesystem can't optimise the copy in some way, they want to know so they can do something else of their own chosing. Hence this seems like the sane option to take here... > 2. Keep it, but change c_f_r to require that both files have a > ->copy_file_range implementation. If they're the same then we'll call > the function pointer, if not, we call the generic fallback. This at > least gets us back to the usual behavior which is that filesystems have > to opt in to new functionality (== we assume they QA'd all the wunnerful > combinations). That doesn't address the "write failure turns into short read" problem with the splice path... > 3. #2, but fix the generic fallback to not suck so badly. That sounds > like someone (else's) 2yr project. :P Not mine, either. Cheers, Dave. -- Dave Chinner email@example.com
next prev parent reply other threads:[~2021-02-15 0:39 UTC|newest] Thread overview: 136+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-02-12 4:43 [PATCH 0/6] Add generated flag to filesystem struct to block copy_file_range Nicolas Boichat 2021-02-12 4:44 ` [PATCH 1/6] fs: Add flag to file_system_type to indicate content is generated Nicolas Boichat 2021-02-12 7:46 ` Greg KH 2021-02-12 8:20 ` Nicolas Boichat 2021-02-12 8:37 ` Greg KH 2021-02-12 15:33 ` Ian Lance Taylor 2021-02-12 15:45 ` Greg KH 2021-02-12 15:59 ` Ian Lance Taylor 2021-02-12 16:28 ` Greg KH 2021-02-12 20:22 ` Ian Lance Taylor 2021-02-12 23:03 ` Dave Chinner 2021-02-12 23:07 ` Ian Lance Taylor 2021-02-12 23:27 ` Dave Chinner 2021-02-12 23:54 ` Darrick J. Wong 2021-02-15 0:38 ` Dave Chinner [this message] 2021-02-15 1:12 ` Ian Lance Taylor 2021-02-15 1:25 ` Nicolas Boichat 2021-02-15 5:56 ` Amir Goldstein 2021-02-15 8:30 ` Greg KH 2021-02-12 8:22 ` Amir Goldstein 2021-02-12 8:39 ` Greg KH 2021-02-12 12:05 ` Luis Henriques 2021-02-12 12:18 ` Greg KH 2021-02-12 12:41 ` Luis Henriques 2021-02-12 14:11 ` Greg KH 2021-02-12 15:01 ` Luis Henriques 2021-02-15 6:12 ` Amir Goldstein 2021-02-15 10:39 ` Luis Henriques 2021-02-15 12:22 ` Luis Henriques 2021-02-15 14:23 ` Amir Goldstein 2021-02-15 14:51 ` Luis Henriques 2021-02-15 15:43 ` [PATCH v2] vfs: prevent copy_file_range to copy across devices Luis Henriques 2021-02-15 16:02 ` Trond Myklebust 2021-02-16 0:25 ` Steve French 2021-02-15 16:34 ` Amir Goldstein 2021-02-15 16:53 ` Trond Myklebust 2021-02-15 17:24 ` Amir Goldstein 2021-02-15 18:57 ` Trond Myklebust 2021-02-15 19:43 ` Amir Goldstein 2021-02-16 11:17 ` Luis Henriques 2021-02-16 11:28 ` gregkh 2021-02-16 12:01 ` Luis Henriques 2021-02-16 12:08 ` Greg KH 2021-02-16 13:51 ` Amir Goldstein 2021-02-16 16:42 ` Luis Henriques 2021-02-16 17:44 ` Amir Goldstein 2021-02-16 18:55 ` Luis Henriques 2021-02-16 19:20 ` Amir Goldstein 2021-02-16 19:27 ` Anna Schumaker 2021-02-16 19:31 ` Steve French 2021-02-16 19:40 ` Amir Goldstein 2021-02-16 21:15 ` Steve French 2021-02-17 8:08 ` Amir Goldstein 2021-02-17 17:26 ` [PATCH v3] vfs: fix copy_file_range regression in cross-fs copies Luis Henriques 2021-02-17 20:47 ` Amir Goldstein 2021-02-18 0:56 ` Nicolas Boichat 2021-02-18 5:32 ` Olga Kornievskaia 2021-02-18 6:47 ` Amir Goldstein 2021-02-18 16:28 ` Olga Kornievskaia 2021-02-18 7:43 ` Christoph Hellwig 2021-02-18 0:50 ` [PATCH v2] vfs: prevent copy_file_range to copy across devices Andreas Dilger 2021-02-18 7:34 ` gregkh 2021-02-16 18:54 ` Andreas Dilger 2021-02-17 4:45 ` Nicolas Boichat 2021-02-18 7:42 ` Christoph Hellwig 2021-02-18 9:10 ` Amir Goldstein 2021-02-18 10:29 ` Luis Henriques 2021-02-18 12:15 ` Luis Henriques 2021-02-18 12:49 ` Amir Goldstein 2021-02-18 14:36 ` [PATCH v4] vfs: fix copy_file_range regression in cross-fs copies Luis Henriques 2021-02-18 14:58 ` Amir Goldstein 2021-02-18 15:17 ` [PATCH v5] " Luis Henriques 2021-02-18 15:53 ` Amir Goldstein 2021-02-18 16:35 ` Luis Henriques 2021-02-18 17:18 ` [PATCH v6] " Luis Henriques 2021-02-19 21:18 ` Olga Kornievskaia 2021-02-19 21:52 ` Amir Goldstein 2021-02-21 19:58 ` [PATCH v7] " Luis Henriques 2021-02-22 3:00 ` Nicolas Boichat 2021-02-22 10:24 ` [PATCH v8] " Luis Henriques 2021-02-22 10:46 ` Amir Goldstein 2021-02-22 16:25 ` dai.ngo 2021-02-23 10:32 ` Luis Henriques 2021-02-23 15:28 ` Amir Goldstein 2021-02-23 15:29 ` dai.ngo 2021-02-23 16:02 ` dai.ngo 2021-02-23 16:47 ` Amir Goldstein 2021-02-23 16:57 ` dai.ngo [not found] ` <firstname.lastname@example.org> 2021-02-23 17:33 ` Amir Goldstein 2021-02-24 0:13 ` dai.ngo 2021-02-23 17:56 ` Luis Henriques 2021-02-23 17:13 ` Olga Kornievskaia 2021-02-24 1:00 ` Olga Kornievskaia 2021-02-24 10:23 ` Luis Henriques 2021-02-24 10:44 ` Nicolas Boichat 2021-04-09 5:23 ` Nicolas Boichat 2021-04-09 13:39 ` Luis Henriques 2021-04-09 13:50 ` Amir Goldstein 2021-04-23 4:40 ` Nicolas Boichat 2021-05-03 8:54 ` Luis Henriques 2021-05-10 4:59 ` Amir Goldstein 2021-05-10 9:10 ` Luis Henriques 2021-02-24 14:23 ` [PATCH] copy_file_range.2: Kernel v5.12 updates Luis Henriques 2021-02-24 16:10 ` Amir Goldstein 2021-02-25 10:21 ` Luis Henriques 2021-02-26 10:13 ` Alejandro Colomar (man-pages) 2021-02-26 10:34 ` Amir Goldstein 2021-02-26 11:15 ` Alejandro Colomar (man-pages) 2021-02-26 13:59 ` Jeff Layton 2021-02-26 21:26 ` Alejandro Colomar (man-pages) 2021-02-26 22:18 ` Alejandro Colomar (man-pages) 2021-02-27 5:41 ` Amir Goldstein 2021-02-27 12:20 ` Alejandro Colomar (man-pages) 2021-02-27 13:49 ` [RFC v2] copy_file_range.2: Update cross-filesystem support for 5.12 Alejandro Colomar 2021-02-27 16:00 ` Amir Goldstein 2021-02-27 23:08 ` [PATCH] copy_file_range.2: Kernel v5.12 updates Steve French 2021-02-28 7:35 ` Amir Goldstein 2021-02-28 22:25 ` Steve French 2021-03-01 6:18 ` Amir Goldstein 2021-03-01 14:41 ` [RFC v3] copy_file_range.2: Update cross-filesystem support for 5.12 Alejandro Colomar 2021-03-01 14:58 ` Amir Goldstein 2021-03-04 9:38 ` [RFC v4] " Alejandro Colomar 2021-03-04 17:13 ` Darrick J. Wong 2021-03-04 18:24 ` Alejandro Colomar (man-pages) 2021-03-04 23:50 ` Darrick J. Wong 2021-02-24 7:15 ` [PATCH v4] vfs: fix copy_file_range regression in cross-fs copies Amir Goldstein 2021-02-24 8:30 ` Petr Vorel 2021-02-18 20:41 ` [PATCH v2] vfs: prevent copy_file_range to copy across devices Steve French 2021-02-12 23:15 ` [PATCH 1/6] fs: Add flag to file_system_type to indicate content is generated Dave Chinner 2021-02-12 7:54 ` Amir Goldstein 2021-02-12 4:44 ` [PATCH 2/6] proc: Add FS_GENERATED_CONTENT to filesystem flags Nicolas Boichat 2021-02-12 4:44 ` [PATCH 6/6] vfs: Disallow copy_file_range on generated file systems Nicolas Boichat 2021-02-12 4:53 ` Darrick J. Wong 2021-02-12 4:59 ` Darrick J. Wong 2021-02-12 5:24 ` Nicolas Boichat 2021-02-14 23:09 ` [PATCH 0/6] Add generated flag to filesystem struct to block copy_file_range Al Viro
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=20210215003855.GY4626@dread.disaster.area \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --subject='Re: [PATCH 1/6] fs: Add flag to file_system_type to indicate content is generated' \ /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: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).