Messages in this thread | | | Subject | Re: [PATCH v2 2/3] xen/privcmd: Mark pages as dirty | From | Jürgen Groß <> | Date | Wed, 8 Jul 2020 07:40:14 +0200 |
| |
On 07.07.20 21:30, John Hubbard wrote: > On 2020-07-07 04:43, Jürgen Groß wrote: >> On 07.07.20 13:30, Souptick Joarder wrote: >>> On Tue, Jul 7, 2020 at 3:08 PM Jürgen Groß <jgross@suse.com> wrote: > ... >>>>> diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c >>>>> index 33677ea..f6c1543 100644 >>>>> --- a/drivers/xen/privcmd.c >>>>> +++ b/drivers/xen/privcmd.c >>>>> @@ -612,8 +612,11 @@ static void unlock_pages(struct page *pages[], >>>>> unsigned int nr_pages) >>>>> { >>>>> unsigned int i; >>>>> >>>>> - for (i = 0; i < nr_pages; i++) >>>>> + for (i = 0; i < nr_pages; i++) { >>>>> + if (!PageDirty(pages[i])) >>>>> + set_page_dirty_lock(pages[i]); >>>> >>>> With put_page() directly following I think you should be able to use >>>> set_page_dirty() instead, as there is obviously a reference to the page >>>> existing. >>> >>> Patch [3/3] will convert above codes to use >>> unpin_user_pages_dirty_lock() >>> which internally do the same check. So I thought to keep linux-stable >>> and >>> linux-next code in sync. John had a similar concern [1] and later >>> agreed to keep >>> this check. >>> >>> Shall I keep this check ? No ? > > It doesn't matter *too* much, because patch 3/3 fixes up everything by > changing it all to unpin_user_pages_dirty_lock(). However, there is > something > to be said for having correct interim patches, too. :) Details: > >>> >>> [1] >>> https://lore.kernel.org/xen-devel/a750e5e5-fd5d-663b-c5fd-261d7c939ba7@nvidia.com/ >>> >> >> I wasn't referring to checking PageDirty(), but to the use of >> set_page_dirty_lock(). >> >> Looking at the comment just before the implementation of >> set_page_dirty_lock() suggests that it is fine to use set_page_dirty() >> instead (so not calling lock_page()). > > > no no, that's a misreading of the comment. Unless this xen/privcmd code has > somehow taken a reference on page->mapping->host (which I do *not* think is > the case), then it is still racy to call set_page_dirty() here. Instead, > set_page_dirty_lock() should be used.
Ah, okay. Thanks for the clarification.
So you can add my
Reviewed-by: Juergen Gross <jgross@suse.com>
Juergen
| |