[PATCH v2 3/3] fbdev: Refactor implementation of page_mkwrite

[Sam Ravnborg]

Hi Thomas.

On Mon, Apr 25, 2022 at 01:27:51PM +0200, Thomas Zimmermann wrote:
> Refactor the page-write handler for deferred I/O. Drivers use the
> function to let fbdev track written pages of mmap'ed framebuffer
> memory.

I like how the comments got a brush up and a little more info was added.
But I do not see the point of the refactoring - the code is equally
readable before and after - maybe even easier before (modulus the
improved comments).

But if you consider it better keep it. Again just my thoughts when
reading the code.

	Sam

> 
> v2:
> 	* don't export the helper until we have an external caller
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
> Reviewed-by: Javier Martinez Canillas <javierm at redhat.com>
> ---
>  drivers/video/fbdev/core/fb_defio.c | 68 ++++++++++++++++++++---------
>  1 file changed, 48 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c
> index a03b9c64fc61..214581ce5840 100644
> --- a/drivers/video/fbdev/core/fb_defio.c
> +++ b/drivers/video/fbdev/core/fb_defio.c
> @@ -143,29 +143,18 @@ int fb_deferred_io_fsync(struct file *file, loff_t start, loff_t end, int datasy
>  }
>  EXPORT_SYMBOL_GPL(fb_deferred_io_fsync);
>  
> -/* vm_ops->page_mkwrite handler */
> -static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
> +/*
> + * Adds a page to the dirty list. Requires caller to hold
> + * struct fb_deferred_io.lock. Call this from struct
> + * vm_operations_struct.page_mkwrite.
> + */
> +static vm_fault_t __fb_deferred_io_track_page(struct fb_info *info, unsigned long offset,
> +					      struct page *page)
>  {
> -	struct page *page = vmf->page;
> -	struct fb_info *info = vmf->vma->vm_private_data;
>  	struct fb_deferred_io *fbdefio = info->fbdefio;
>  	struct fb_deferred_io_pageref *pageref;
> -	unsigned long offset;
>  	vm_fault_t ret;
>  
> -	offset = (vmf->address - vmf->vma->vm_start);
> -
> -	/* this is a callback we get when userspace first tries to
> -	write to the page. we schedule a workqueue. that workqueue
> -	will eventually mkclean the touched pages and execute the
> -	deferred framebuffer IO. then if userspace touches a page
> -	again, we repeat the same scheme */
> -
> -	file_update_time(vmf->vma->vm_file);
> -
> -	/* protect against the workqueue changing the page list */
> -	mutex_lock(&fbdefio->lock);
> -
>  	/* first write in this cycle, notify the driver */
>  	if (fbdefio->first_io && list_empty(&fbdefio->pagelist))
>  		fbdefio->first_io(info);
> @@ -186,8 +175,6 @@ static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
>  	 */
>  	lock_page(pageref->page);
>  
> -	mutex_unlock(&fbdefio->lock);
> -
>  	/* come back after delay to process the deferred IO */
>  	schedule_delayed_work(&info->deferred_work, fbdefio->delay);
>  	return VM_FAULT_LOCKED;
> @@ -197,6 +184,47 @@ static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
>  	return ret;
>  }
>  
> +/*
> + * fb_deferred_io_page_mkwrite - Mark a page as written for deferred I/O
> + * @fb_info: The fbdev info structure
> + * @vmf: The VM fault
> + *
> + * This is a callback we get when userspace first tries to
> + * write to the page. We schedule a workqueue. That workqueue
> + * will eventually mkclean the touched pages and execute the
> + * deferred framebuffer IO. Then if userspace touches a page
> + * again, we repeat the same scheme.
> + *
> + * Returns:
> + * VM_FAULT_LOCKED on success, or a VM_FAULT error otherwise.
> + */
> +static vm_fault_t fb_deferred_io_page_mkwrite(struct fb_info *info, struct vm_fault *vmf)
> +{
> +	struct page *page = vmf->page;
> +	struct fb_deferred_io *fbdefio = info->fbdefio;
> +	unsigned long offset;
> +	vm_fault_t ret;
> +
> +	offset = (vmf->address - vmf->vma->vm_start);
> +
> +	file_update_time(vmf->vma->vm_file);
> +
> +	/* protect against the workqueue changing the page list */
> +	mutex_lock(&fbdefio->lock);
> +	ret = __fb_deferred_io_track_page(info, offset, page);
> +	mutex_unlock(&fbdefio->lock);
> +
> +	return ret;
> +}
> +
> +/* vm_ops->page_mkwrite handler */
> +static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
> +{
> +	struct fb_info *info = vmf->vma->vm_private_data;
> +
> +	return fb_deferred_io_page_mkwrite(info, vmf);
> +}
> +
>  static const struct vm_operations_struct fb_deferred_io_vm_ops = {
>  	.fault		= fb_deferred_io_fault,
>  	.page_mkwrite	= fb_deferred_io_mkwrite,
> -- 
> 2.36.0