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

[Christoph Hellwig <hch@infradead.org>]

> +	/* 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.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>