Re: [PATCH v5 4/5] cramfs: add mmap support

From: Christoph Hellwig
Date: Fri Oct 06 2017 - 03:00:51 EST


> + /* Don't map the last page if it contains some other data */
> + if (unlikely(pgoff + pages == max_pages)) {
> + unsigned int partial = offset_in_page(inode->i_size);
> + if (partial) {
> + char *data = sbi->linear_virt_addr + offset;
> + data += (max_pages - 1) * PAGE_SIZE + partial;
> + if (memchr_inv(data, 0, PAGE_SIZE - partial) != NULL) {
> + pr_debug("mmap: %s: last page is shared\n",
> + file_dentry(file)->d_name.name);
> + pages--;
> + }
> + }
> + }

Why is pgoff + pages == max_pages marked unlikely? Mapping the whole
file seems like a perfectly normal and likely case to me..

Also if this was my code I'd really prefer to move this into a helper:

static bool cramfs_mmap_last_page_is_shared(struct inode *inode, int offset)
{
unsigned int partial = offset_in_page(inode->i_size);
char *data = CRAMFS_SB(inode->i_sb)->linear_virt_addr + offset +
(inode->i_size & PAGE_MASK);

return memchr_inv(data + partial, 0, PAGE_SIZE - partial);
}

if (pgoff + pages == max_pages && offset_in_page(inode->i_size) &&
cramfs_mmap_last_page_is_shared(inode, offset))
pages--;

as that's much more readable and the function name provides a good
documentation of what is going on.

> + if (pages != vma_pages(vma)) {

here is how I would turn this around:

if (!pages)
goto done;

if (pages == vma_pages(vma)) {
remap_pfn_range();
goto done;
}

...
for (i = 0; i < pages; i++) {
...
vm_insert_mixed();
nr_mapped++;
}


done:
pr_debug("mapped %d out ouf %d\n", ..);
if (pages != vma_pages(vma))
vma->vm_ops = &generic_file_vm_ops;
return 0;
}

In fact we probably could just set the vm_ops unconditionally, they
just wouldn't be called, but that might be more confusing then helpful.