msync - can someone shed some light please?

[Julien ÉLIE]

Hi Richard,

> Assuming the page size is 4096 bytes:
> * p = 0x7f3f46800000 is already page-aligned so start = p
> * length = 5062656 is a multiple of 4096,
>    so we will get end = p + length + 4096,
>    given end-start = length+4096.
> 
> The effect is that INN will attempt to msync 4096 bytes beyond the end 
> of the mapping.
> 
> In https://man7.org/linux/man-pages/man2/msync.2.html doing this is 
> documented as being an error. The error code is consistent with the log 
> message:
> 
>         ENOMEM The indicated memory (or part of it) was not mapped.
> 
> If length % 4096 > 0 then the effect would just be to round up to an 
> exact number of pages, avoiding the error. So we'd expect to see the 
> error in 1/4096 (about 0.025%) of possible active files sizes.
> 
> Anyway I think it should be:
> 
>    char *end = (char *) (((uintptr_t) p + length + pagesize - 1) & mask);
> 
> ...so that we end up to the round of the allocation, and never any further.

Oh, thanks for having had a look and the patch proposal.  (Thanks, Bo, 
too for the confirmation.)

This function is also widely used by tradindexed, as well as some 
history writes when nfswriter is set to true in inn.conf.  So your patch 
will also fix latent bugs for these cases too.

Looking at the whole source code, there's also cnfs_mapcntl, a slight 
variant of the above function in storage/cnfs/cnfs.c, which saves msync 
calls when the pages are the same as the previous ones.
     start = (char *) ((uintptr_t) p & ~(size_t) (pagesize - 1));
     end = (char *) (((uintptr_t) p + length + pagesize)
                     & ~(size_t) (pagesize - 1));

end should then be fixed to use (p + length + pagesize - 1), following 
your patch proposal.

And in frontends/cnfsstat.in:
         my $pagesize = 16384;
         my $minartoffset = int($len / ($blksz * 8)) + 512;
         # Align upwards:
         $minartoffset = ($minartoffset + $pagesize) & ~($pagesize - 1);

though I do not think there's an actual bug in cnfsstat, it's for 
reading chunks of data and it would possibly just save 1 read.

-- 
Julien ÉLIE

« C'est une forêt vierge où la main de l'homme n'a jamais mis le pied. »