diff mbox series

[v3,2/3] dax: Add a wakeup mode parameter to put_unlocked_entry()

Message ID 20210419213636.1514816-3-vgoyal@redhat.com (mailing list archive)
State Superseded
Headers show
Series dax: Fix missed wakeup in put_unlocked_entry() | expand

Commit Message

Vivek Goyal April 19, 2021, 9:36 p.m. UTC
As of now put_unlocked_entry() always wakes up next waiter. In next
patches we want to wake up all waiters at one callsite. Hence, add a
parameter to the function.

This patch does not introduce any change of behavior.

Suggested-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/dax.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

Comments

Greg Kurz April 20, 2021, 7:34 a.m. UTC | #1
On Mon, 19 Apr 2021 17:36:35 -0400
Vivek Goyal <vgoyal@redhat.com> wrote:

> As of now put_unlocked_entry() always wakes up next waiter. In next
> patches we want to wake up all waiters at one callsite. Hence, add a
> parameter to the function.
> 
> This patch does not introduce any change of behavior.
> 
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/dax.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/dax.c b/fs/dax.c
> index 00978d0838b1..f19d76a6a493 100644
> --- a/fs/dax.c
> +++ b/fs/dax.c
> @@ -275,11 +275,12 @@ static void wait_entry_unlocked(struct xa_state *xas, void *entry)
>  	finish_wait(wq, &ewait.wait);
>  }
>  
> -static void put_unlocked_entry(struct xa_state *xas, void *entry)
> +static void put_unlocked_entry(struct xa_state *xas, void *entry,
> +			       enum dax_entry_wake_mode mode)
>  {
>  	/* If we were the only waiter woken, wake the next one */

With this change, the comment is no longer accurate since the
function can now wake all waiters if passed mode == WAKE_ALL.
Also, it paraphrases the code which is simple enough, so I'd
simply drop it.

This is minor though and it shouldn't prevent this fix to go
forward.

Reviewed-by: Greg Kurz <groug@kaod.org>

>  	if (entry && !dax_is_conflict(entry))
> -		dax_wake_entry(xas, entry, WAKE_NEXT);
> +		dax_wake_entry(xas, entry, mode);
>  }
>  
>  /*
> @@ -633,7 +634,7 @@ struct page *dax_layout_busy_page_range(struct address_space *mapping,
>  			entry = get_unlocked_entry(&xas, 0);
>  		if (entry)
>  			page = dax_busy_page(entry);
> -		put_unlocked_entry(&xas, entry);
> +		put_unlocked_entry(&xas, entry, WAKE_NEXT);
>  		if (page)
>  			break;
>  		if (++scanned % XA_CHECK_SCHED)
> @@ -675,7 +676,7 @@ static int __dax_invalidate_entry(struct address_space *mapping,
>  	mapping->nrexceptional--;
>  	ret = 1;
>  out:
> -	put_unlocked_entry(&xas, entry);
> +	put_unlocked_entry(&xas, entry, WAKE_NEXT);
>  	xas_unlock_irq(&xas);
>  	return ret;
>  }
> @@ -954,7 +955,7 @@ static int dax_writeback_one(struct xa_state *xas, struct dax_device *dax_dev,
>  	return ret;
>  
>   put_unlocked:
> -	put_unlocked_entry(xas, entry);
> +	put_unlocked_entry(xas, entry, WAKE_NEXT);
>  	return ret;
>  }
>  
> @@ -1695,7 +1696,7 @@ dax_insert_pfn_mkwrite(struct vm_fault *vmf, pfn_t pfn, unsigned int order)
>  	/* Did we race with someone splitting entry or so? */
>  	if (!entry || dax_is_conflict(entry) ||
>  	    (order == 0 && !dax_is_pte_entry(entry))) {
> -		put_unlocked_entry(&xas, entry);
> +		put_unlocked_entry(&xas, entry, WAKE_NEXT);
>  		xas_unlock_irq(&xas);
>  		trace_dax_insert_pfn_mkwrite_no_entry(mapping->host, vmf,
>  						      VM_FAULT_NOPAGE);
Vivek Goyal April 20, 2021, 2 p.m. UTC | #2
On Tue, Apr 20, 2021 at 09:34:20AM +0200, Greg Kurz wrote:
> On Mon, 19 Apr 2021 17:36:35 -0400
> Vivek Goyal <vgoyal@redhat.com> wrote:
> 
> > As of now put_unlocked_entry() always wakes up next waiter. In next
> > patches we want to wake up all waiters at one callsite. Hence, add a
> > parameter to the function.
> > 
> > This patch does not introduce any change of behavior.
> > 
> > Suggested-by: Dan Williams <dan.j.williams@intel.com>
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  fs/dax.c | 13 +++++++------
> >  1 file changed, 7 insertions(+), 6 deletions(-)
> > 
> > diff --git a/fs/dax.c b/fs/dax.c
> > index 00978d0838b1..f19d76a6a493 100644
> > --- a/fs/dax.c
> > +++ b/fs/dax.c
> > @@ -275,11 +275,12 @@ static void wait_entry_unlocked(struct xa_state *xas, void *entry)
> >  	finish_wait(wq, &ewait.wait);
> >  }
> >  
> > -static void put_unlocked_entry(struct xa_state *xas, void *entry)
> > +static void put_unlocked_entry(struct xa_state *xas, void *entry,
> > +			       enum dax_entry_wake_mode mode)
> >  {
> >  	/* If we were the only waiter woken, wake the next one */
> 
> With this change, the comment is no longer accurate since the
> function can now wake all waiters if passed mode == WAKE_ALL.
> Also, it paraphrases the code which is simple enough, so I'd
> simply drop it.
> 
> This is minor though and it shouldn't prevent this fix to go
> forward.
> 
> Reviewed-by: Greg Kurz <groug@kaod.org>

Ok, here is the updated patch which drops that comment line.

Vivek

Subject: dax: Add a wakeup mode parameter to put_unlocked_entry()

As of now put_unlocked_entry() always wakes up next waiter. In next
patches we want to wake up all waiters at one callsite. Hence, add a
parameter to the function.

This patch does not introduce any change of behavior.

Suggested-by: Dan Williams <dan.j.williams@intel.com>
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/dax.c |   14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

Index: redhat-linux/fs/dax.c
===================================================================
--- redhat-linux.orig/fs/dax.c	2021-04-20 09:55:45.105069893 -0400
+++ redhat-linux/fs/dax.c	2021-04-20 09:56:27.685822730 -0400
@@ -275,11 +275,11 @@ static void wait_entry_unlocked(struct x
 	finish_wait(wq, &ewait.wait);
 }
 
-static void put_unlocked_entry(struct xa_state *xas, void *entry)
+static void put_unlocked_entry(struct xa_state *xas, void *entry,
+			       enum dax_entry_wake_mode mode)
 {
-	/* If we were the only waiter woken, wake the next one */
 	if (entry && !dax_is_conflict(entry))
-		dax_wake_entry(xas, entry, WAKE_NEXT);
+		dax_wake_entry(xas, entry, mode);
 }
 
 /*
@@ -633,7 +633,7 @@ struct page *dax_layout_busy_page_range(
 			entry = get_unlocked_entry(&xas, 0);
 		if (entry)
 			page = dax_busy_page(entry);
-		put_unlocked_entry(&xas, entry);
+		put_unlocked_entry(&xas, entry, WAKE_NEXT);
 		if (page)
 			break;
 		if (++scanned % XA_CHECK_SCHED)
@@ -675,7 +675,7 @@ static int __dax_invalidate_entry(struct
 	mapping->nrexceptional--;
 	ret = 1;
 out:
-	put_unlocked_entry(&xas, entry);
+	put_unlocked_entry(&xas, entry, WAKE_NEXT);
 	xas_unlock_irq(&xas);
 	return ret;
 }
@@ -954,7 +954,7 @@ static int dax_writeback_one(struct xa_s
 	return ret;
 
  put_unlocked:
-	put_unlocked_entry(xas, entry);
+	put_unlocked_entry(xas, entry, WAKE_NEXT);
 	return ret;
 }
 
@@ -1695,7 +1695,7 @@ dax_insert_pfn_mkwrite(struct vm_fault *
 	/* Did we race with someone splitting entry or so? */
 	if (!entry || dax_is_conflict(entry) ||
 	    (order == 0 && !dax_is_pte_entry(entry))) {
-		put_unlocked_entry(&xas, entry);
+		put_unlocked_entry(&xas, entry, WAKE_NEXT);
 		xas_unlock_irq(&xas);
 		trace_dax_insert_pfn_mkwrite_no_entry(mapping->host, vmf,
 						      VM_FAULT_NOPAGE);
Jan Kara April 21, 2021, 9:25 a.m. UTC | #3
On Mon 19-04-21 17:36:35, Vivek Goyal wrote:
> As of now put_unlocked_entry() always wakes up next waiter. In next
> patches we want to wake up all waiters at one callsite. Hence, add a
> parameter to the function.
> 
> This patch does not introduce any change of behavior.
> 
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

Looks good. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/dax.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/dax.c b/fs/dax.c
> index 00978d0838b1..f19d76a6a493 100644
> --- a/fs/dax.c
> +++ b/fs/dax.c
> @@ -275,11 +275,12 @@ static void wait_entry_unlocked(struct xa_state *xas, void *entry)
>  	finish_wait(wq, &ewait.wait);
>  }
>  
> -static void put_unlocked_entry(struct xa_state *xas, void *entry)
> +static void put_unlocked_entry(struct xa_state *xas, void *entry,
> +			       enum dax_entry_wake_mode mode)
>  {
>  	/* If we were the only waiter woken, wake the next one */
>  	if (entry && !dax_is_conflict(entry))
> -		dax_wake_entry(xas, entry, WAKE_NEXT);
> +		dax_wake_entry(xas, entry, mode);
>  }
>  
>  /*
> @@ -633,7 +634,7 @@ struct page *dax_layout_busy_page_range(struct address_space *mapping,
>  			entry = get_unlocked_entry(&xas, 0);
>  		if (entry)
>  			page = dax_busy_page(entry);
> -		put_unlocked_entry(&xas, entry);
> +		put_unlocked_entry(&xas, entry, WAKE_NEXT);
>  		if (page)
>  			break;
>  		if (++scanned % XA_CHECK_SCHED)
> @@ -675,7 +676,7 @@ static int __dax_invalidate_entry(struct address_space *mapping,
>  	mapping->nrexceptional--;
>  	ret = 1;
>  out:
> -	put_unlocked_entry(&xas, entry);
> +	put_unlocked_entry(&xas, entry, WAKE_NEXT);
>  	xas_unlock_irq(&xas);
>  	return ret;
>  }
> @@ -954,7 +955,7 @@ static int dax_writeback_one(struct xa_state *xas, struct dax_device *dax_dev,
>  	return ret;
>  
>   put_unlocked:
> -	put_unlocked_entry(xas, entry);
> +	put_unlocked_entry(xas, entry, WAKE_NEXT);
>  	return ret;
>  }
>  
> @@ -1695,7 +1696,7 @@ dax_insert_pfn_mkwrite(struct vm_fault *vmf, pfn_t pfn, unsigned int order)
>  	/* Did we race with someone splitting entry or so? */
>  	if (!entry || dax_is_conflict(entry) ||
>  	    (order == 0 && !dax_is_pte_entry(entry))) {
> -		put_unlocked_entry(&xas, entry);
> +		put_unlocked_entry(&xas, entry, WAKE_NEXT);
>  		xas_unlock_irq(&xas);
>  		trace_dax_insert_pfn_mkwrite_no_entry(mapping->host, vmf,
>  						      VM_FAULT_NOPAGE);
> -- 
> 2.25.4
>
Vivek Goyal April 21, 2021, 5:39 p.m. UTC | #4
On Tue, Apr 20, 2021 at 09:34:20AM +0200, Greg Kurz wrote:
> On Mon, 19 Apr 2021 17:36:35 -0400
> Vivek Goyal <vgoyal@redhat.com> wrote:
> 
> > As of now put_unlocked_entry() always wakes up next waiter. In next
> > patches we want to wake up all waiters at one callsite. Hence, add a
> > parameter to the function.
> > 
> > This patch does not introduce any change of behavior.
> > 
> > Suggested-by: Dan Williams <dan.j.williams@intel.com>
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  fs/dax.c | 13 +++++++------
> >  1 file changed, 7 insertions(+), 6 deletions(-)
> > 
> > diff --git a/fs/dax.c b/fs/dax.c
> > index 00978d0838b1..f19d76a6a493 100644
> > --- a/fs/dax.c
> > +++ b/fs/dax.c
> > @@ -275,11 +275,12 @@ static void wait_entry_unlocked(struct xa_state *xas, void *entry)
> >  	finish_wait(wq, &ewait.wait);
> >  }
> >  
> > -static void put_unlocked_entry(struct xa_state *xas, void *entry)
> > +static void put_unlocked_entry(struct xa_state *xas, void *entry,
> > +			       enum dax_entry_wake_mode mode)
> >  {
> >  	/* If we were the only waiter woken, wake the next one */
> 
> With this change, the comment is no longer accurate since the
> function can now wake all waiters if passed mode == WAKE_ALL.
> Also, it paraphrases the code which is simple enough, so I'd
> simply drop it.

Ok, I will get rid of this comment. Agreed that code is simple
enough. And frankly speaking I don't even understand "If we were the
only waiter woken" part. How do we know that only this caller
was woken.

Vivek

> 
> This is minor though and it shouldn't prevent this fix to go
> forward.
> 
> Reviewed-by: Greg Kurz <groug@kaod.org>
> 
> >  	if (entry && !dax_is_conflict(entry))
> > -		dax_wake_entry(xas, entry, WAKE_NEXT);
> > +		dax_wake_entry(xas, entry, mode);
> >  }
> >  
> >  /*
> > @@ -633,7 +634,7 @@ struct page *dax_layout_busy_page_range(struct address_space *mapping,
> >  			entry = get_unlocked_entry(&xas, 0);
> >  		if (entry)
> >  			page = dax_busy_page(entry);
> > -		put_unlocked_entry(&xas, entry);
> > +		put_unlocked_entry(&xas, entry, WAKE_NEXT);
> >  		if (page)
> >  			break;
> >  		if (++scanned % XA_CHECK_SCHED)
> > @@ -675,7 +676,7 @@ static int __dax_invalidate_entry(struct address_space *mapping,
> >  	mapping->nrexceptional--;
> >  	ret = 1;
> >  out:
> > -	put_unlocked_entry(&xas, entry);
> > +	put_unlocked_entry(&xas, entry, WAKE_NEXT);
> >  	xas_unlock_irq(&xas);
> >  	return ret;
> >  }
> > @@ -954,7 +955,7 @@ static int dax_writeback_one(struct xa_state *xas, struct dax_device *dax_dev,
> >  	return ret;
> >  
> >   put_unlocked:
> > -	put_unlocked_entry(xas, entry);
> > +	put_unlocked_entry(xas, entry, WAKE_NEXT);
> >  	return ret;
> >  }
> >  
> > @@ -1695,7 +1696,7 @@ dax_insert_pfn_mkwrite(struct vm_fault *vmf, pfn_t pfn, unsigned int order)
> >  	/* Did we race with someone splitting entry or so? */
> >  	if (!entry || dax_is_conflict(entry) ||
> >  	    (order == 0 && !dax_is_pte_entry(entry))) {
> > -		put_unlocked_entry(&xas, entry);
> > +		put_unlocked_entry(&xas, entry, WAKE_NEXT);
> >  		xas_unlock_irq(&xas);
> >  		trace_dax_insert_pfn_mkwrite_no_entry(mapping->host, vmf,
> >  						      VM_FAULT_NOPAGE);
>
Dan Williams April 21, 2021, 7:09 p.m. UTC | #5
On Tue, Apr 20, 2021 at 7:01 AM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Tue, Apr 20, 2021 at 09:34:20AM +0200, Greg Kurz wrote:
> > On Mon, 19 Apr 2021 17:36:35 -0400
> > Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> > > As of now put_unlocked_entry() always wakes up next waiter. In next
> > > patches we want to wake up all waiters at one callsite. Hence, add a
> > > parameter to the function.
> > >
> > > This patch does not introduce any change of behavior.
> > >
> > > Suggested-by: Dan Williams <dan.j.williams@intel.com>
> > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > > ---
> > >  fs/dax.c | 13 +++++++------
> > >  1 file changed, 7 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/fs/dax.c b/fs/dax.c
> > > index 00978d0838b1..f19d76a6a493 100644
> > > --- a/fs/dax.c
> > > +++ b/fs/dax.c
> > > @@ -275,11 +275,12 @@ static void wait_entry_unlocked(struct xa_state *xas, void *entry)
> > >     finish_wait(wq, &ewait.wait);
> > >  }
> > >
> > > -static void put_unlocked_entry(struct xa_state *xas, void *entry)
> > > +static void put_unlocked_entry(struct xa_state *xas, void *entry,
> > > +                          enum dax_entry_wake_mode mode)
> > >  {
> > >     /* If we were the only waiter woken, wake the next one */
> >
> > With this change, the comment is no longer accurate since the
> > function can now wake all waiters if passed mode == WAKE_ALL.
> > Also, it paraphrases the code which is simple enough, so I'd
> > simply drop it.
> >
> > This is minor though and it shouldn't prevent this fix to go
> > forward.
> >
> > Reviewed-by: Greg Kurz <groug@kaod.org>
>
> Ok, here is the updated patch which drops that comment line.
>
> Vivek

Hi Vivek,

Can you get in the habit of not replying inline with new patches like
this? Collect the review feedback, take a pause, and resend the full
series so tooling like b4 and patchwork can track when a new posting
supersedes a previous one. As is, this inline style inflicts manual
effort on the maintainer.

>
> Subject: dax: Add a wakeup mode parameter to put_unlocked_entry()
>
> As of now put_unlocked_entry() always wakes up next waiter. In next
> patches we want to wake up all waiters at one callsite. Hence, add a
> parameter to the function.
>
> This patch does not introduce any change of behavior.
>
> Suggested-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/dax.c |   14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
>
> Index: redhat-linux/fs/dax.c
> ===================================================================
> --- redhat-linux.orig/fs/dax.c  2021-04-20 09:55:45.105069893 -0400
> +++ redhat-linux/fs/dax.c       2021-04-20 09:56:27.685822730 -0400
> @@ -275,11 +275,11 @@ static void wait_entry_unlocked(struct x
>         finish_wait(wq, &ewait.wait);
>  }
>
> -static void put_unlocked_entry(struct xa_state *xas, void *entry)
> +static void put_unlocked_entry(struct xa_state *xas, void *entry,
> +                              enum dax_entry_wake_mode mode)
>  {
> -       /* If we were the only waiter woken, wake the next one */
>         if (entry && !dax_is_conflict(entry))
> -               dax_wake_entry(xas, entry, WAKE_NEXT);
> +               dax_wake_entry(xas, entry, mode);
>  }
>
>  /*
> @@ -633,7 +633,7 @@ struct page *dax_layout_busy_page_range(
>                         entry = get_unlocked_entry(&xas, 0);
>                 if (entry)
>                         page = dax_busy_page(entry);
> -               put_unlocked_entry(&xas, entry);
> +               put_unlocked_entry(&xas, entry, WAKE_NEXT);
>                 if (page)
>                         break;
>                 if (++scanned % XA_CHECK_SCHED)
> @@ -675,7 +675,7 @@ static int __dax_invalidate_entry(struct
>         mapping->nrexceptional--;
>         ret = 1;
>  out:
> -       put_unlocked_entry(&xas, entry);
> +       put_unlocked_entry(&xas, entry, WAKE_NEXT);
>         xas_unlock_irq(&xas);
>         return ret;
>  }
> @@ -954,7 +954,7 @@ static int dax_writeback_one(struct xa_s
>         return ret;
>
>   put_unlocked:
> -       put_unlocked_entry(xas, entry);
> +       put_unlocked_entry(xas, entry, WAKE_NEXT);
>         return ret;
>  }
>
> @@ -1695,7 +1695,7 @@ dax_insert_pfn_mkwrite(struct vm_fault *
>         /* Did we race with someone splitting entry or so? */
>         if (!entry || dax_is_conflict(entry) ||
>             (order == 0 && !dax_is_pte_entry(entry))) {
> -               put_unlocked_entry(&xas, entry);
> +               put_unlocked_entry(&xas, entry, WAKE_NEXT);
>                 xas_unlock_irq(&xas);
>                 trace_dax_insert_pfn_mkwrite_no_entry(mapping->host, vmf,
>                                                       VM_FAULT_NOPAGE);
>
Vivek Goyal April 21, 2021, 7:13 p.m. UTC | #6
On Wed, Apr 21, 2021 at 12:09:54PM -0700, Dan Williams wrote:
> On Tue, Apr 20, 2021 at 7:01 AM Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> > On Tue, Apr 20, 2021 at 09:34:20AM +0200, Greg Kurz wrote:
> > > On Mon, 19 Apr 2021 17:36:35 -0400
> > > Vivek Goyal <vgoyal@redhat.com> wrote:
> > >
> > > > As of now put_unlocked_entry() always wakes up next waiter. In next
> > > > patches we want to wake up all waiters at one callsite. Hence, add a
> > > > parameter to the function.
> > > >
> > > > This patch does not introduce any change of behavior.
> > > >
> > > > Suggested-by: Dan Williams <dan.j.williams@intel.com>
> > > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > > > ---
> > > >  fs/dax.c | 13 +++++++------
> > > >  1 file changed, 7 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/fs/dax.c b/fs/dax.c
> > > > index 00978d0838b1..f19d76a6a493 100644
> > > > --- a/fs/dax.c
> > > > +++ b/fs/dax.c
> > > > @@ -275,11 +275,12 @@ static void wait_entry_unlocked(struct xa_state *xas, void *entry)
> > > >     finish_wait(wq, &ewait.wait);
> > > >  }
> > > >
> > > > -static void put_unlocked_entry(struct xa_state *xas, void *entry)
> > > > +static void put_unlocked_entry(struct xa_state *xas, void *entry,
> > > > +                          enum dax_entry_wake_mode mode)
> > > >  {
> > > >     /* If we were the only waiter woken, wake the next one */
> > >
> > > With this change, the comment is no longer accurate since the
> > > function can now wake all waiters if passed mode == WAKE_ALL.
> > > Also, it paraphrases the code which is simple enough, so I'd
> > > simply drop it.
> > >
> > > This is minor though and it shouldn't prevent this fix to go
> > > forward.
> > >
> > > Reviewed-by: Greg Kurz <groug@kaod.org>
> >
> > Ok, here is the updated patch which drops that comment line.
> >
> > Vivek
> 
> Hi Vivek,
> 
> Can you get in the habit of not replying inline with new patches like
> this? Collect the review feedback, take a pause, and resend the full
> series so tooling like b4 and patchwork can track when a new posting
> supersedes a previous one. As is, this inline style inflicts manual
> effort on the maintainer.

Hi Dan,

Sure. I will avoid doing this updated inline patch style. I will post new
version of patch series. 

Thanks
Vivek

> 
> >
> > Subject: dax: Add a wakeup mode parameter to put_unlocked_entry()
> >
> > As of now put_unlocked_entry() always wakes up next waiter. In next
> > patches we want to wake up all waiters at one callsite. Hence, add a
> > parameter to the function.
> >
> > This patch does not introduce any change of behavior.
> >
> > Suggested-by: Dan Williams <dan.j.williams@intel.com>
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  fs/dax.c |   14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > Index: redhat-linux/fs/dax.c
> > ===================================================================
> > --- redhat-linux.orig/fs/dax.c  2021-04-20 09:55:45.105069893 -0400
> > +++ redhat-linux/fs/dax.c       2021-04-20 09:56:27.685822730 -0400
> > @@ -275,11 +275,11 @@ static void wait_entry_unlocked(struct x
> >         finish_wait(wq, &ewait.wait);
> >  }
> >
> > -static void put_unlocked_entry(struct xa_state *xas, void *entry)
> > +static void put_unlocked_entry(struct xa_state *xas, void *entry,
> > +                              enum dax_entry_wake_mode mode)
> >  {
> > -       /* If we were the only waiter woken, wake the next one */
> >         if (entry && !dax_is_conflict(entry))
> > -               dax_wake_entry(xas, entry, WAKE_NEXT);
> > +               dax_wake_entry(xas, entry, mode);
> >  }
> >
> >  /*
> > @@ -633,7 +633,7 @@ struct page *dax_layout_busy_page_range(
> >                         entry = get_unlocked_entry(&xas, 0);
> >                 if (entry)
> >                         page = dax_busy_page(entry);
> > -               put_unlocked_entry(&xas, entry);
> > +               put_unlocked_entry(&xas, entry, WAKE_NEXT);
> >                 if (page)
> >                         break;
> >                 if (++scanned % XA_CHECK_SCHED)
> > @@ -675,7 +675,7 @@ static int __dax_invalidate_entry(struct
> >         mapping->nrexceptional--;
> >         ret = 1;
> >  out:
> > -       put_unlocked_entry(&xas, entry);
> > +       put_unlocked_entry(&xas, entry, WAKE_NEXT);
> >         xas_unlock_irq(&xas);
> >         return ret;
> >  }
> > @@ -954,7 +954,7 @@ static int dax_writeback_one(struct xa_s
> >         return ret;
> >
> >   put_unlocked:
> > -       put_unlocked_entry(xas, entry);
> > +       put_unlocked_entry(xas, entry, WAKE_NEXT);
> >         return ret;
> >  }
> >
> > @@ -1695,7 +1695,7 @@ dax_insert_pfn_mkwrite(struct vm_fault *
> >         /* Did we race with someone splitting entry or so? */
> >         if (!entry || dax_is_conflict(entry) ||
> >             (order == 0 && !dax_is_pte_entry(entry))) {
> > -               put_unlocked_entry(&xas, entry);
> > +               put_unlocked_entry(&xas, entry, WAKE_NEXT);
> >                 xas_unlock_irq(&xas);
> >                 trace_dax_insert_pfn_mkwrite_no_entry(mapping->host, vmf,
> >                                                       VM_FAULT_NOPAGE);
> >
>
Christoph Hellwig April 22, 2021, 6:24 a.m. UTC | #7
On Wed, Apr 21, 2021 at 12:09:54PM -0700, Dan Williams wrote:
> Can you get in the habit of not replying inline with new patches like
> this? Collect the review feedback, take a pause, and resend the full
> series so tooling like b4 and patchwork can track when a new posting
> supersedes a previous one. As is, this inline style inflicts manual
> effort on the maintainer.

Honestly I don't mind it at all.  If you shiny new tooling can't handle
it maybe you should fix your shiny new tooling instead of changing
everyones workflow?
Darrick J. Wong April 22, 2021, 4:12 p.m. UTC | #8
On Thu, Apr 22, 2021 at 07:24:58AM +0100, Christoph Hellwig wrote:
> On Wed, Apr 21, 2021 at 12:09:54PM -0700, Dan Williams wrote:
> > Can you get in the habit of not replying inline with new patches like
> > this? Collect the review feedback, take a pause, and resend the full
> > series so tooling like b4 and patchwork can track when a new posting
> > supersedes a previous one. As is, this inline style inflicts manual
> > effort on the maintainer.
> 
> Honestly I don't mind it at all.  If you shiny new tooling can't handle
> it maybe you should fix your shiny new tooling instead of changing
> everyones workflow?

Just speaking for XFS here, but I don't like inline resubmissions
because that makes it /really/ hard to find the original patch 6 months
later when everything has paged out of my brain but random enterprise
distro backporters start asking questions ("is this an actively
exploited security fix?" "what were you smoking?" etc).

At least change the subject line to something that screams "new patch!"
so that mutt and lore will make it stand out.

(Granted this isn't XFS, so I am not the enforcer here ;))

--D
Dan Williams April 22, 2021, 8:01 p.m. UTC | #9
On Wed, Apr 21, 2021 at 11:25 PM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Wed, Apr 21, 2021 at 12:09:54PM -0700, Dan Williams wrote:
> > Can you get in the habit of not replying inline with new patches like
> > this? Collect the review feedback, take a pause, and resend the full
> > series so tooling like b4 and patchwork can track when a new posting
> > supersedes a previous one. As is, this inline style inflicts manual
> > effort on the maintainer.
>
> Honestly I don't mind it at all.  If you shiny new tooling can't handle
> it maybe you should fix your shiny new tooling instead of changing
> everyones workflow?

I think asking a submitter to resend a series is par for the course,
especially for poor saps like me burdened by corporate email systems.
Vivek, if this is too onerous a request just give me a heads up and
I'll manually pull out the patch content from your replies.
Vivek Goyal April 22, 2021, 8:07 p.m. UTC | #10
On Thu, Apr 22, 2021 at 01:01:15PM -0700, Dan Williams wrote:
> On Wed, Apr 21, 2021 at 11:25 PM Christoph Hellwig <hch@infradead.org> wrote:
> >
> > On Wed, Apr 21, 2021 at 12:09:54PM -0700, Dan Williams wrote:
> > > Can you get in the habit of not replying inline with new patches like
> > > this? Collect the review feedback, take a pause, and resend the full
> > > series so tooling like b4 and patchwork can track when a new posting
> > > supersedes a previous one. As is, this inline style inflicts manual
> > > effort on the maintainer.
> >
> > Honestly I don't mind it at all.  If you shiny new tooling can't handle
> > it maybe you should fix your shiny new tooling instead of changing
> > everyones workflow?
> 
> I think asking a submitter to resend a series is par for the course,
> especially for poor saps like me burdened by corporate email systems.
> Vivek, if this is too onerous a request just give me a heads up and
> I'll manually pull out the patch content from your replies.

I am fine with posting new version. Initially I thought that there
were only 1-2 minor cleanup comments so I posted inline, thinking
it might preferred method instead of posting full patch series again.

But then more comments came along. So posting another version makes
more sense now.

Thanks
Vivek
diff mbox series

Patch

diff --git a/fs/dax.c b/fs/dax.c
index 00978d0838b1..f19d76a6a493 100644
--- a/fs/dax.c
+++ b/fs/dax.c
@@ -275,11 +275,12 @@  static void wait_entry_unlocked(struct xa_state *xas, void *entry)
 	finish_wait(wq, &ewait.wait);
 }
 
-static void put_unlocked_entry(struct xa_state *xas, void *entry)
+static void put_unlocked_entry(struct xa_state *xas, void *entry,
+			       enum dax_entry_wake_mode mode)
 {
 	/* If we were the only waiter woken, wake the next one */
 	if (entry && !dax_is_conflict(entry))
-		dax_wake_entry(xas, entry, WAKE_NEXT);
+		dax_wake_entry(xas, entry, mode);
 }
 
 /*
@@ -633,7 +634,7 @@  struct page *dax_layout_busy_page_range(struct address_space *mapping,
 			entry = get_unlocked_entry(&xas, 0);
 		if (entry)
 			page = dax_busy_page(entry);
-		put_unlocked_entry(&xas, entry);
+		put_unlocked_entry(&xas, entry, WAKE_NEXT);
 		if (page)
 			break;
 		if (++scanned % XA_CHECK_SCHED)
@@ -675,7 +676,7 @@  static int __dax_invalidate_entry(struct address_space *mapping,
 	mapping->nrexceptional--;
 	ret = 1;
 out:
-	put_unlocked_entry(&xas, entry);
+	put_unlocked_entry(&xas, entry, WAKE_NEXT);
 	xas_unlock_irq(&xas);
 	return ret;
 }
@@ -954,7 +955,7 @@  static int dax_writeback_one(struct xa_state *xas, struct dax_device *dax_dev,
 	return ret;
 
  put_unlocked:
-	put_unlocked_entry(xas, entry);
+	put_unlocked_entry(xas, entry, WAKE_NEXT);
 	return ret;
 }
 
@@ -1695,7 +1696,7 @@  dax_insert_pfn_mkwrite(struct vm_fault *vmf, pfn_t pfn, unsigned int order)
 	/* Did we race with someone splitting entry or so? */
 	if (!entry || dax_is_conflict(entry) ||
 	    (order == 0 && !dax_is_pte_entry(entry))) {
-		put_unlocked_entry(&xas, entry);
+		put_unlocked_entry(&xas, entry, WAKE_NEXT);
 		xas_unlock_irq(&xas);
 		trace_dax_insert_pfn_mkwrite_no_entry(mapping->host, vmf,
 						      VM_FAULT_NOPAGE);