Re: [PATCH v2 05/21] Add a function to kmap one page of a multipage bio_vec

Christoph Hellwig <[email protected]> wrote:

> > +static inline void *kmap_local_bvec(struct bio_vec *bvec, size_t offset)
> 
> The name is rather confusing for something that does not map the entire
> bvec, and is an anagram of the existing bvec_kmap_local.  So please
> rename it to bvec_kmap_partial or something.

Sure...  But it then differs in pattern from the various kmap_local_*() that
we have.

> > +{
> > +	offset += bvec->bv_offset;
> > +
> > +	return kmap_local_page(bvec->bv_page + offset / PAGE_SIZE) + offset % PAGE_SIZE;
> 
> ...  Also this can use shits and byte masking to be a tad more efficient and
> matching the rest of the bvec code.

PAGE_SIZE is a constant power-of-2, so it shouldn't make a difference if the
compiler is doing its job properly, but okay.

One thought: I could just give bvec_kmap_local() an offset parameter and use
that instead.

> Users of this would be interesting

It's used four times in patch 10 ("afs: Use a bvecq to hold dir content rather
than folioq").  I could merge this patch (patch 5) into one of the other
patches, it's just been more convenient for it not be in a later patch when
flipping backwards and forwards to cut down on recompiling.

> and why you're not simply using a bvec_iter at page granularity, which is
> what other block kmap code does.

Because for the most part I don't want to do linear iteration.  There are a
number of cases:

 (*) Read-link and follow-link: For symlinks, there will only be a single
     block and it will fit within PAGE_SIZE.

 (*) Readdir and dir checking: I iterate over the directory contents using
     iov_iter and iterate_bvecq(), though I could also use bvec_iter inside a
     loop to step through the bvecq chain.

 (*) Lookup: Follows the hash chain in the directory rather than searching
     linearly.

 (*) Editing of local directory copy: Doesn't do linear iteration, but rather
     does random access of blocks in the directory.  (This is to avoid having
     to reload the dir from the server when doing mkdir, rename, etc.)

Note that directories are 2K, not PAGE_SIZE, and, when editing, I may need to
modify more than one block in a directory operation and they may share a page
(which may be part of a higher order page).  Can we drop 32-bit arches yet -
or, at least, highem support?

David