Subject: Re: genfs_getpages() nit
To: Bill Studenmund <wrstuden@zembu.com>
From: Jaromír Dolecek <dolecek@ics.muni.cz>
List: tech-kern
Date: 01/27/2001 11:24:08
> > -	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.

> >  	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 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 ?
* 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, ...)

Jaromir
-- 
Jaromir Dolecek <jdolecek@NetBSD.org>      http://www.ics.muni.cz/~dolecek/
@@@@  Wanna a real operating system ? Go and get NetBSD, dammit!  @@@@