On Mon, Jan 24, 2022 at 04:25:11PM +0000, David Howells wrote:
> Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote:
>
> > Well, that's the problem isn't it? You're expecting to mutate the state
> > and then refer to its previous state instead of its current state,
>
> No. I *don't* want to see the previous state. That's where the problem is:
> The previous state isn't cleared up until the point I ask for a new batch -
> but I need to query the ractl to find the remaining window before I can ask
> for a new batch.
Oh, right, even worse -- you want to know the _future_ state of the
iterator before you mutate it (it's kind of the same thing though
if you haven't looked at how the iterator works internally).
> > whereas the other users refer to the current state instead of the
> > previous state.
>
> Fuse has exactly the same issues:
... and yet you refuse to solve it the same way as fuse ...
> > Can't you pull readahead_index() out of the ractl ahead of the mutation?
>
> I'm not sure what you mean by that. What I'm now doing is:
>
> while ((nr_pages = readahead_count(ractl) - ractl->_batch_count)) {
> ...
> pgoff_t index = readahead_index(ractl) + ractl->_batch_count;
> ...
> rc = cifs_fscache_query_occupancy(
> ractl->mapping->host, index, nr_pages,
> ...
> rc = server->ops->wait_mtu_credits(server, cifs_sb->ctx->rsize,
> &rsize, credits);
> ...
> nr_pages = min_t(size_t, rsize / PAGE_SIZE, readahead_count(ractl));
> nr_pages = min_t(size_t, nr_pages, next_cached - index);
> ...
> rdata = cifs_readdata_alloc(nr_pages, cifs_readv_complete);
> ...
> got = __readahead_batch(ractl, rdata->pages, nr_pages);
> ...
> }
>
> I need the count to know how many, if any, pages are remaining; I need the
> count and index of the window to find out if fscache has any data; I need the
> count to allocate a page array. Only after all that can I crank the next
> batch for the server (assuming I didn't delegate to the cache along the way,
> but that calls readahead_page()).
The problem is that the other users very much want to refer to the
pre-mutation state. eg, btrfs:
while ((nr = readahead_page_batch(rac, pagepool))) {
u64 contig_start = readahead_pos(rac);
u64 contig_end = contig_start + readahead_batch_length(rac) - 1;
contiguous_readpages(pagepool, nr, contig_start, contig_end,
&em_cached, &bio_ctrl, &prev_em_start);
}
The API isn't designed to work the way you want it to work. So either
we redesign it (... and change all the existing users ...) or you use
it the way that FUSE does, which is admittedly suboptimal, but you're
also saying it's temporary.
--
Linux-cachefs mailing list
Linux-cachefs@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/linux-cachefs
