On Sat, Jan 27, 2001 at 11:24:08AM +0100, r Dolecek wrote:
> > > -	endoffset = round_page((origoffset + (npages << PAGE_SHIFT)
> > > -				+ fs_bsize - 1) & ~(fs_bsize - 1));
> > > +	if (fs_bshift < PAGE_SHIFT)
> > > +		endoffset = round_page(startoffset + (npages << PAGE_SHIFT));
> > > +	else {
> > > +		endoffset = round_page(startoffset +
> > > +		  (((npages << PAGE_SHIFT) + fs_bsize - 1) & ~(fs_bsize - 1)));
> > > +	}
> > 
> > "I'm-still-confused"
> 
> If I read the code correctly, this code tries to make endoffset rounded to both
> PAGE_SIZE and fs_bsize. If fs_bshift is < PAGE_SHIFT, we don't need
> to count with fs_bsize, since it would not have any influence on
> end result. Also, we can use startoffset instead of origoffset, since
> that is already rounded to fs_bsize.

since most of the filesystems out there are going to be 8k blocksize FFS
on a machines with 8k or less pagesize, we're going to end up in the
more complicated expression most of the time anyway.  checking for
for blocksize vs. pagesize will just slow down the common case.


> > >  	endoffset = min(endoffset, round_page(eof));
> > > -	ridx = (origoffset - startoffset) >> PAGE_SHIFT;
> > > +	ridx = (origoffset & fs_bsize) >> PAGE_SHIFT;
> > 
> > Shouldn't that be ridx = (origoffset & (fs_bsize - 1)) >> PAGE_SHIFT;
> > ?
> 
> Ah, true. I'm not sure which one is more readable then (the old one,
> or this new one).

I like the existing one better.


> I also realized two other thigs which might be "interesting" from
> looking at genfs_getpages(). Maybe I'm just confused (which is quite likely ;)
> 
> * does mmap() work if filesystem block size is >= 16 * PAGE_SIZE in UBC
>   in all cases ?

blocksize = 16 * pagesize stands a good chance of working, but I haven't tried.
blocksize > 16 * pagesize will not work.  but that didn't work before anyway.


> * I'm not sure if the code in genfs_getpages() below uvm_pagermapin() 
>   (line approx. 628) is okay for block size > PAGE_SIZE; shouldn't
>   the offset ridx be used there as well similarily to code above it ?
>   i.e. like
> 	kva = uvm_pagermapin(&pgs[ridx], npages, ...)

it's right the way it is now, since the "if (startoffset != origoffset) {..}"
block a bit before that that switches the origin within the pgs array
from ridx to 0.  there are some other bugs in genfs_getpages() where npages
is wrong since it doesn't take the origin-switching into account,
but I'm testing fixes for those now.

-Chuck