On Mon, May 01 2006, Oleg Nesterov wrote:
> On 05/01, Jens Axboe wrote:
> >
> > On Mon, May 01 2006, Oleg Nesterov wrote:
> > >
> > > I can't understand why do we need PIPE_BUF_FLAG_STOLEN at all.
> > > It seems to me we need a local boolean in pipe_to_file.
> >
> > PIPE_BUF_FLAG_STOLEN used to be used in the release function as well,
> > hence the flag.
>
> Ok, but in that case
>
> > I'll make sure to clear
> > the flag as well on add_to_page_cache() failure.
>
> ... it is not good to clear it in pipe_to_file(). The page remains
> stolen from pipe_buf_operations pov, this flag imho should be private
> to buf, and page_cache_pipe_buf_ops doesn't need it.
>
> I think pipe_to_buf() can test 'buf->page == page' instead of
> PIPE_BUF_FLAG_STOLEN.
I ended up fixing it with a local variable, but you are right it can be
killed with just a buf->page != page == stolen check. I got rid of the
last check of that, so just one remaining. Will commit this change.
> Another question,
>
> __generic_file_splice_read:
>
> /*
> * Initiate read-ahead on this page range. however, don't call into
> * read-ahead if this is a non-zero offset (we are likely doing small
> * chunk splice and the page is already there) for a single page.
> */
> if (!loff || nr_pages > 1)
> page_cache_readahead(mapping, &in->f_ra, in, index, nr_pages);
>
> Why this check? page_cache_readahead() should detect sub-page
> reads correctly.
Leftover from do_page_cache_readahead I suppose. I probably shouldn't
try to second guess read-ahead, however.
> page = find_get_page(mapping, index);
> if (!page) {
> page = page_cache_alloc_cold();
>
> add_to_page_cache_lru(page);
>
> I think it makes sense to add handle_ra_miss() here. Otherwise,
> for example, readahead could be disabled by RA_FLAG_INCACHE
> forever.
Good point, added.
> If readahead doesn't work, SPLICE_F_MOVE is problematic too.
> add_to_page_cache_lru()->lru_cache_add() first increments
> page->count and adds this page to lru_add_pvecs. This means
> page_cache_pipe_buf_steal()->remove_mapping() will probably
> fail.
Because of the temporarily elevated page count?
--
Jens Axboe
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
