lkml.org 
[lkml]   [2020]   [Jun]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v7 1/8] fs: introduce kernel_pread_file* support
    From
    Date
    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

    \
     
     \ /
      Last update: 2020-06-10 00:55    [W:4.028 / U:0.020 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site