Hi Thomas,
On Thu, Feb 10, 2022 at 03:11:11PM +0100, Thomas Zimmermann wrote:
> Return early if a page is already in the list of dirty pages for
> deferred I/O. This can be detected if the page's list head is not
> empty. Keep the list head initialized while the page is not enlisted
> to make this work reliably.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
> ---
> drivers/video/fbdev/core/fb_defio.c | 21 ++++++++++++---------
> 1 file changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c
> index a591d291b231..3727b1ca87b1 100644
> --- a/drivers/video/fbdev/core/fb_defio.c
> +++ b/drivers/video/fbdev/core/fb_defio.c
> @@ -59,6 +59,7 @@ static vm_fault_t fb_deferred_io_fault(struct vm_fault *vmf)
> printk(KERN_ERR "no mapping available\n");
>
> BUG_ON(!page->mapping);
> + INIT_LIST_HEAD(&page->lru);
> page->index = vmf->pgoff;
>
> vmf->page = page;
> @@ -122,17 +123,19 @@ static vm_fault_t fb_deferred_io_mkwrite(struct vm_fault *vmf)
> */
> lock_page(page);
>
> + /*
> + * This check is to catch the case where a new process could start
> + * writing to the same page through a new pte. this new access
> + * can cause the mkwrite even when the original ps's pte is marked
> + * writable.
> + */
When moving this comment it would be prudent to also fix the wording a
bit.
- Capital in start of sentence and after a period
- Spell out process and do not shorten ps
> + if (!list_empty(&page->lru))
> + goto page_already_added;
> +
This check says that if the page already has something in the parge->lru
then this is added by defio and thus is already added.
This matches your commit description - OK.
Maybe add something like:
* Pages added will have their lru set, and it is clered again in the
* deferred work handler.
> /* we loop through the pagelist before adding in order
> to keep the pagelist sorted */
> list_for_each_entry(cur, &fbdefio->pagelist, lru) {
> - /* this check is to catch the case where a new
> - process could start writing to the same page
> - through a new pte. this new access can cause the
> - mkwrite even when the original ps's pte is marked
> - writable */
> - if (unlikely(cur == page))
> - goto page_already_added;
> - else if (cur->index > page->index)
> + if (cur->index > page->index)
> break;
> }
>
> @@ -194,7 +197,7 @@ static void fb_deferred_io_work(struct work_struct *work)
>
> /* clear the list */
> list_for_each_safe(node, next, &fbdefio->pagelist) {
> - list_del(node);
> + list_del_init(node);
> }
> mutex_unlock(&fbdefio->lock);
> }
With the comment adjusted as you see fit
Acked-by: Sam Ravnborg <sam@xxxxxxxxxxxx>
