On Wed, Oct 11, 2017 at 10:05:58PM +0200, Jan Kara wrote:
> Implement a function that filesystems can call to finish handling of
> synchronous page faults. It takes care of syncing appropriare file range
> and insertion of page table entry.
>
> Signed-off-by: Jan Kara <jack@xxxxxxx>
Looks fine except for a few nitpicks below:
Reviewed-by: Christoph Hellwig <hch@xxxxxx>
> + spin_lock_irq(&mapping->tree_lock);
> + entry = get_unlocked_mapping_entry(mapping, index, &slot);
> + /* Did we race with someone splitting entry or so? */
> + if (!entry || (pe_size == PE_SIZE_PTE && !dax_is_pte_entry(entry)) ||
> + (pe_size == PE_SIZE_PMD && !dax_is_pmd_entry(entry))) {
Minor nitpick:
Maybe keep each of the three conditions on a separate line to make it a
little easier to read.
> + * This function calls fdatasync() on the range of file touched by the page
> + * fault and handles inserting of appropriate page table entry.
Nitpick: we don't call fdatasync, as that doesn't even exist in the
kernel, but vfs_fsync_range. But documenting which functions we call
doesn't seem all that useful to start with, so if you really want to
document it I'd say something like:
"Ensures that the file range touched by the page fault is stored
persistently on the media"
or similar.
> + int err;
> + loff_t start = ((loff_t)vmf->pgoff) << PAGE_SHIFT;
> + size_t len = 0;
> +
> + if (pe_size == PE_SIZE_PTE)
> + len = PAGE_SIZE;
> +#ifdef CONFIG_FS_DAX_PMD
> + else if (pe_size == PE_SIZE_PMD)
> + len = PMD_SIZE;
> +#endif
Is there really any point to ifdef out this single branch?
> + else
> + WARN_ON_ONCE(1);
> + err = vfs_fsync_range(vmf->vma->vm_file, start, start + len - 1, 1);
(this just reminds me that I hate the vfs_fsync_range /->fsync calling
conventions.., offset + len would be way to easy)
