[lkml]   [2006]   [Mar]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [PATCH][RFC] splice support

    (going over the other comments, thanks a lot for taking a good look

    On Wed, Mar 29 2006, Andrew Morton wrote:
    > - Please don't call it `len'. VFS has to deal with "lengths" which can
    > be in units of PAGE_CACHE_SIZE, fs blocksize, 512-bytes sectors or bytes,
    > and it gets confusing. Our liking for variable names like `len' and
    > `count' just makes it worse.
    > If it's in units of pages then call it `npages'. If it's in bytes then
    > call it `nbytes'.

    Hmm well I usually use 'len' or something like that when it's a base
    unit, like bytes. If it was in units of pages or something I agree it
    would be confusing.

    > What _is_ it in units of, anyway? I guess bytes, since it's size_t.
    > I assume all this lenning:
    > unsigned int this_len;
    > this_len = buf->len;
    > if (this_len > len)
    > this_len = len;
    > is dealing with bytes too. You'll be wanting a size_t in there.

    But buf->len is unsigned int to begin with.

    > - I think the `size_t left' in do_splice_to() can overflow if f_pos is
    > sufficiently different from i_size.

    They're both loff_t.

    > - In pipe_to_file():
    > - Shouldn't it be using GFP_HIGHUSER()?
    > - local variable `index' should be unsigned long or, for clarity
    > value, pgoff_t.


    > - Incoming arg `pos' should be loff_t?


    > - It's racy against truncate(). After running ->readpage and
    > lock_page(), need to check for page->mapping == NULL.

    Indeed, fixed.

    > - There's a duplicate flush_dcache_page().

    Doh, fixed.

    > - Why does it run write_one_page()??? (Don't tell me. I'll work it
    > out when I see the commented version ;))

    Can probably go, will re-check!

    > - I worry a bit about the assumption in one place that a non-zero
    > return from commit_write() indicates an error, whereas another place
    > assumes that a negative return is an error. We had problems in the
    > past where some a_ops implementations decided to return small positive
    > numbers from prepare_write() or commit_write() a_ops, which broke
    > stuff. They shouldn't be doing that now, but it's a thing to watch out
    > for.

    I'll just check for < 0 and be safe.

    > - Bug. If write_one_page() returned an error, it still unlocked the page.

    Ok, I guess that could be a future but (it can't fail with !wait now).

    > - In pipe_to_sendpage():
    > - local variable `offset' is ulong, but elsewhere you've used uint.
    > The latter is better.


    > - Again, incoming arg `len' is confusing. I _think_ it's actually
    > "number of bytes to be moved from this page". A comment which explains
    > these things would be nice, and perhaps a better name (bytes_to_send?)

    It's bytes, yes. Comment added.

    > - Should incoming arg `pos' be loff_t? That would give it some meaning.

    Yes changed with the pipe_to_file() change since it modified the actor
    typedef anyways.

    > - Why does it use PAGE_SIZE and PAGE_SHIFT rather than PAGE_CACHE_*?

    Fixed, the other places used PAGE_CACHE_*.

    > - In generic_file_splice_read():
    > - nonatomic modification of f_pos. Is i_mutex held? (see
    > generic_file_llseek())


    > - Darnit, we carried `flags' this far and ended up not using it.
    > (What _does_ flags do, anyway? Reads on..)

    It'll be passed to the actor next! Will probably change some of the
    actor args into a little struct instead of passing so many variables.

    > - In __generic_file_splice_read():
    > - local variable `index' is ulong, could be pgoff_t (for clarity)


    > - local variable `offset' could be uint (it is uint elsewhere, and
    > might generate better code).


    > - local variable `pages' could be uint (but watch out for overflow!!).


    > A better name might be nr_pages (matches find_get_pages()). Then,
    > local variable `array' can be renamed to `pages', which is all much
    > better.

    Agree, fixed.

    > - whoa. We move the pages into the pipe while they're still under
    > read I/O. Is that deliberate? (pls add nice comment).

    It's fine, you need to call ->map() to get at it anways, which will lock
    the page.

    > - These pages can get truncated at any time they're unlocked. Does
    > the code cope with all that?

    I guess page_cache_pipe_buf_map() needs the same ->mapping check?

    > - hm. What happens if the pages which find_get_pages() returned are
    > not contiguous in pagecache? I think your `pages' array gets all
    > jumbled up.

    Hmm please expand.

    > - In move_to_pipe()
    > - Suggest you rename `pages' to `nr_pages', `array' to `pages'. And
    > `i' to `page_nr'.

    Done (except 'i').

    > - local var `bufs' could be renamed `nrbufs' to align with
    > pipe_inode_info and could be made uint.
    > - Do we actually need local var `bufs'? It seems to be caching info->nrbufs.

    Cleaner that way, imho.

    > - release_pages() might be faster than one-at-a-time page_cache_release()

    We should not hit that case very often. Not sure how to handle the
    'cold' right now, so I'll just leave it.

    Jens Axboe

    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to
    More majordomo info at
    Please read the FAQ at

     \ /
      Last update: 2006-03-30 09:47    [W:0.030 / U:9.120 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site