On Fri, 26 Jan 2001, Jarommr Dolecek wrote:

> Hi,
> while looking around on how mmap() internally works, I've came
> across the not very easily parsable stuff in sys/miscfs/genfs/genfs_vnops.c
> at lines 527+. I adjusted it a little so that the intent is more
> easy to grok, this even yielded an microoptimization. Does
> following change look correct ?

One style nit, one is-that-right question, and one I'm-still-confused. :-)

> Index: genfs_vnops.c
> ===================================================================
> RCS file: /cvsroot/syssrc/sys/miscfs/genfs/genfs_vnops.c,v
> retrieving revision 1.25
> diff -u -r1.25 genfs_vnops.c
> --- genfs_vnops.c	2001/01/22 16:39:54	1.25
> +++ genfs_vnops.c	2001/01/26 22:31:11
> @@ -524,17 +524,22 @@
>  	dev_bsize = 1 << dev_bshift;
>  	KASSERT((eof & (dev_bsize - 1)) == 0);
>  
> -	orignpages = min(*ap->a_count,
> -	    round_page(eof - origoffset) >> PAGE_SHIFT);
> -	if (flags & PGO_PASTEOF) {
> +	if (flags & PGO_PASTEOF)
>  		orignpages = *ap->a_count;
> +	else {
> +		orignpages = min(*ap->a_count,
> +			round_page(eof - origoffset) >> PAGE_SHIFT);
>  	}

Looks fine (and actually better), but (style not): could you keep the true
statement in {}'s? e.g. 
if (flags & PGO_PASTEOF) { 
	... 
} else {
  ?

>  	npages = orignpages;
>  	startoffset = origoffset & ~(fs_bsize - 1);
> -	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"

>  	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;
?

>  
>  	memset(pgs, 0, sizeof(pgs));
>  	uvn_findpages(uobj, origoffset, &npages, &pgs[ridx], UFP_ALL);

Take care,

Bill