Hi Matthew,
On 2020-06-09 6:21 a.m., Matthew Wilcox wrote:
On Mon, Jun 08, 2020 at 03:29:22PM -0700, Scott Branden wrote:
Hi Matthew,
I am requesting the experts in the filesystem subsystem to come to a consensus here. This is not my area of expertise at all but every time I have addressed all of the outstanding concerns someone else comes along and raises another one.
I appreciate it's frustrating for you, but this is the nature of patch review. I haven't even read the first five or so submissions. I can see them in my inbox and they look like long threads. I'm not particularly inclined to read them. I happened to read v6, and reacted to the API being ugly.
Thanks for the review. Yes, I do see the enum being ugly now and have removed it in v8 of the patch. Hopefully it addresses your concerns. More comments below.
Please see me comments below.
On 2020-06-06 8:52 a.m., Matthew Wilcox wrote:
On Fri, Jun 05, 2020 at 10:04:51PM -0700, Scott Branden wrote:
-int kernel_read_file(struct file *file, void **buf, loff_t *size,
loff_t max_size, enum kernel_read_file_id id)-{
- loff_t i_size, pos;
Please note that how checkpatch generated the diff here. The code modifications below are for a new function kernel_pread_file, they do not modify the existing API kernel_read_file. kernel_read_file requests the ENTIRE file is read. So we need to be able to differentiate whether it is ok to read just a portion of the file or not.
You've gone about this in entirely the wrong way though. This enum to read the entire file or a partial is just bad design.
Your point on the enum is valid. I've removed it from design. Hopefully it is cleaner now.
+int kernel_pread_file(struct file *file, void **buf, loff_t *size,
loff_t pos, loff_t max_size,enum kernel_pread_opt opt,enum kernel_read_file_id id)So, to share common code a new kernel_pread_opt needed to be added in order to specify whether it was ok to read a partial file or not, and provide an offset into the file where to begin reading. The meaning of parameters doesn't change in the bonkers API. max_size still means max size, etc. These options are needed so common code can be shared with kernel_read_file api.
Does pread() in userspace take seven parameters? No. It takes four. What you're doing is taking all the complexity of all of the interfaces and stuffing it all down into the bottom function instead of handling some of the complexity in the wrapper functions. For example, you could support the functionality of 'max_size' in kernel_read_file() and leave it out of the kernel_pread_file() interface.
I have removed the enum necessary in the kernel pread call now, so it is down to 6. The other 2 parameters are necessary as they are in kernel read.
max_size makes no sense to remove - it serves the same purpose as in userspace pread and read functions. To specify the max size to read.
I think what we actually want is:
ssize_t vmap_file_range(struct file *, loff_t start, loff_t end, void **bufp); void vunmap_file_range(struct file *, void *buf);
If end > i_size, limit the allocation to i_size. Returns the number of bytes allocated, or a negative errno. Writes the pointer allocated to *bufp. Internally, it should use the page cache to read in the pages (taking appropriate reference counts). Then it maps them using vmap() instead of copying them to a private vmalloc() array. kernel_read_file() can be converted to use this API. The users will need to be changed to call kernel_read_end(struct file *file, void *buf) instead of vfree() so it can call allow_write_access() for them.
vmap_file_range() has a lot of potential uses. I'm surprised we don't have it already, to be honest.
Such a change sounds like it could be done in a later patch series. It's an incomplete solution. It would work for some of the needed operations but not others. For kernel_read_file, I don't see how in your new API it indicates if the end of the file was reached or not.
That's the point. It doesn't. If a caller needs that, then they can figure that out themselves.
No, they can't. The caller only calls kernel_read_file once and expects the whole file to be read. The kernel_read_file doesn't work like userspace. There is no tracking like userspace of where in the file you read?
Also, please note that buffers may be preallocated and shouldn't be freed by the kernel in some cases and allocated and freed by the kernel in others.
You're trying to build the swiss army knife of functions. Swiss army knives are useful, but they're no good for carving a steak.
Hopefully I'm carving steak now.
I would like the experts here to decide on what needs to be done so we can move forward and get kernel_pread_file support added soon.
You know, you haven't even said _why_ you want this. The cover letter just says "I want this", and doesn't say why it's needed.
Cover letter updated.
Thanks, Scott