| Message ID | 20210419213636.1514816-2-vgoyal@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dax: Fix missed wakeup in put_unlocked_entry() | expand |
On Mon, 19 Apr 2021 17:36:34 -0400 Vivek Goyal <vgoyal@redhat.com> wrote: > Dan mentioned that he is not very fond of passing around a boolean true/false > to specify if only next waiter should be woken up or all waiters should be > woken up. He instead prefers that we introduce an enum and make it very > explicity at the callsite itself. Easier to read code. > > This patch should not introduce any change of behavior. > > Suggested-by: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com> > --- Reviewed-by: Greg Kurz <groug@kaod.org> > fs/dax.c | 23 +++++++++++++++++------ > 1 file changed, 17 insertions(+), 6 deletions(-) > > diff --git a/fs/dax.c b/fs/dax.c > index b3d27fdc6775..00978d0838b1 100644 > --- a/fs/dax.c > +++ b/fs/dax.c > @@ -144,6 +144,16 @@ struct wait_exceptional_entry_queue { > struct exceptional_entry_key key; > }; > > +/** > + * enum dax_entry_wake_mode: waitqueue wakeup toggle > + * @WAKE_NEXT: entry was not mutated > + * @WAKE_ALL: entry was invalidated, or resized > + */ > +enum dax_entry_wake_mode { > + WAKE_NEXT, > + WAKE_ALL, > +}; > + > static wait_queue_head_t *dax_entry_waitqueue(struct xa_state *xas, > void *entry, struct exceptional_entry_key *key) > { > @@ -182,7 +192,8 @@ static int wake_exceptional_entry_func(wait_queue_entry_t *wait, > * The important information it's conveying is whether the entry at > * this index used to be a PMD entry. > */ > -static void dax_wake_entry(struct xa_state *xas, void *entry, bool wake_all) > +static void dax_wake_entry(struct xa_state *xas, void *entry, > + enum dax_entry_wake_mode mode) > { > struct exceptional_entry_key key; > wait_queue_head_t *wq; > @@ -196,7 +207,7 @@ static void dax_wake_entry(struct xa_state *xas, void *entry, bool wake_all) > * must be in the waitqueue and the following check will see them. > */ > if (waitqueue_active(wq)) > - __wake_up(wq, TASK_NORMAL, wake_all ? 0 : 1, &key); > + __wake_up(wq, TASK_NORMAL, mode == WAKE_ALL ? 0 : 1, &key); > } > > /* > @@ -268,7 +279,7 @@ static void put_unlocked_entry(struct xa_state *xas, void *entry) > { > /* If we were the only waiter woken, wake the next one */ > if (entry && !dax_is_conflict(entry)) > - dax_wake_entry(xas, entry, false); > + dax_wake_entry(xas, entry, WAKE_NEXT); > } > > /* > @@ -286,7 +297,7 @@ static void dax_unlock_entry(struct xa_state *xas, void *entry) > old = xas_store(xas, entry); > xas_unlock_irq(xas); > BUG_ON(!dax_is_locked(old)); > - dax_wake_entry(xas, entry, false); > + dax_wake_entry(xas, entry, WAKE_NEXT); > } > > /* > @@ -524,7 +535,7 @@ static void *grab_mapping_entry(struct xa_state *xas, > > dax_disassociate_entry(entry, mapping, false); > xas_store(xas, NULL); /* undo the PMD join */ > - dax_wake_entry(xas, entry, true); > + dax_wake_entry(xas, entry, WAKE_ALL); > mapping->nrexceptional--; > entry = NULL; > xas_set(xas, index); > @@ -937,7 +948,7 @@ static int dax_writeback_one(struct xa_state *xas, struct dax_device *dax_dev, > xas_lock_irq(xas); > xas_store(xas, entry); > xas_clear_mark(xas, PAGECACHE_TAG_DIRTY); > - dax_wake_entry(xas, entry, false); > + dax_wake_entry(xas, entry, WAKE_NEXT); > > trace_dax_writeback_one(mapping->host, index, count); > return ret;
On Mon 19-04-21 17:36:34, Vivek Goyal wrote: > Dan mentioned that he is not very fond of passing around a boolean true/false > to specify if only next waiter should be woken up or all waiters should be > woken up. He instead prefers that we introduce an enum and make it very > explicity at the callsite itself. Easier to read code. > > This patch should 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 | 23 +++++++++++++++++------ > 1 file changed, 17 insertions(+), 6 deletions(-) > > diff --git a/fs/dax.c b/fs/dax.c > index b3d27fdc6775..00978d0838b1 100644 > --- a/fs/dax.c > +++ b/fs/dax.c > @@ -144,6 +144,16 @@ struct wait_exceptional_entry_queue { > struct exceptional_entry_key key; > }; > > +/** > + * enum dax_entry_wake_mode: waitqueue wakeup toggle > + * @WAKE_NEXT: entry was not mutated > + * @WAKE_ALL: entry was invalidated, or resized Let's document the constants in terms of what they do, not when they are expected to be called. So something like: @WAKE_NEXT: wake only the first waiter in the waitqueue @WAKE_ALL: wake all waiters in the waitqueue Otherwise the patch looks good so feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> Honza > + */ > +enum dax_entry_wake_mode { > + WAKE_NEXT, > + WAKE_ALL, > +}; > + > static wait_queue_head_t *dax_entry_waitqueue(struct xa_state *xas, > void *entry, struct exceptional_entry_key *key) > { > @@ -182,7 +192,8 @@ static int wake_exceptional_entry_func(wait_queue_entry_t *wait, > * The important information it's conveying is whether the entry at > * this index used to be a PMD entry. > */ > -static void dax_wake_entry(struct xa_state *xas, void *entry, bool wake_all) > +static void dax_wake_entry(struct xa_state *xas, void *entry, > + enum dax_entry_wake_mode mode) > { > struct exceptional_entry_key key; > wait_queue_head_t *wq; > @@ -196,7 +207,7 @@ static void dax_wake_entry(struct xa_state *xas, void *entry, bool wake_all) > * must be in the waitqueue and the following check will see them. > */ > if (waitqueue_active(wq)) > - __wake_up(wq, TASK_NORMAL, wake_all ? 0 : 1, &key); > + __wake_up(wq, TASK_NORMAL, mode == WAKE_ALL ? 0 : 1, &key); > } > > /* > @@ -268,7 +279,7 @@ static void put_unlocked_entry(struct xa_state *xas, void *entry) > { > /* If we were the only waiter woken, wake the next one */ > if (entry && !dax_is_conflict(entry)) > - dax_wake_entry(xas, entry, false); > + dax_wake_entry(xas, entry, WAKE_NEXT); > } > > /* > @@ -286,7 +297,7 @@ static void dax_unlock_entry(struct xa_state *xas, void *entry) > old = xas_store(xas, entry); > xas_unlock_irq(xas); > BUG_ON(!dax_is_locked(old)); > - dax_wake_entry(xas, entry, false); > + dax_wake_entry(xas, entry, WAKE_NEXT); > } > > /* > @@ -524,7 +535,7 @@ static void *grab_mapping_entry(struct xa_state *xas, > > dax_disassociate_entry(entry, mapping, false); > xas_store(xas, NULL); /* undo the PMD join */ > - dax_wake_entry(xas, entry, true); > + dax_wake_entry(xas, entry, WAKE_ALL); > mapping->nrexceptional--; > entry = NULL; > xas_set(xas, index); > @@ -937,7 +948,7 @@ static int dax_writeback_one(struct xa_state *xas, struct dax_device *dax_dev, > xas_lock_irq(xas); > xas_store(xas, entry); > xas_clear_mark(xas, PAGECACHE_TAG_DIRTY); > - dax_wake_entry(xas, entry, false); > + dax_wake_entry(xas, entry, WAKE_NEXT); > > trace_dax_writeback_one(mapping->host, index, count); > return ret; > -- > 2.25.4 >
On Wed, Apr 21, 2021 at 11:24:40AM +0200, Jan Kara wrote: > On Mon 19-04-21 17:36:34, Vivek Goyal wrote: > > Dan mentioned that he is not very fond of passing around a boolean true/false > > to specify if only next waiter should be woken up or all waiters should be > > woken up. He instead prefers that we introduce an enum and make it very > > explicity at the callsite itself. Easier to read code. > > > > This patch should 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 | 23 +++++++++++++++++------ > > 1 file changed, 17 insertions(+), 6 deletions(-) > > > > diff --git a/fs/dax.c b/fs/dax.c > > index b3d27fdc6775..00978d0838b1 100644 > > --- a/fs/dax.c > > +++ b/fs/dax.c > > @@ -144,6 +144,16 @@ struct wait_exceptional_entry_queue { > > struct exceptional_entry_key key; > > }; > > > > +/** > > + * enum dax_entry_wake_mode: waitqueue wakeup toggle > > + * @WAKE_NEXT: entry was not mutated > > + * @WAKE_ALL: entry was invalidated, or resized > > Let's document the constants in terms of what they do, not when they are > expected to be called. So something like: > > @WAKE_NEXT: wake only the first waiter in the waitqueue > @WAKE_ALL: wake all waiters in the waitqueue > > Otherwise the patch looks good so feel free to add: > > Reviewed-by: Jan Kara <jack@suse.cz> > Hi Jan, Here is the updated patch based on your feedback. Thanks Vivek Subject: dax: Add an enum for specifying dax wakup mode Dan mentioned that he is not very fond of passing around a boolean true/false to specify if only next waiter should be woken up or all waiters should be woken up. He instead prefers that we introduce an enum and make it very explicity at the callsite itself. Easier to read code. This patch should 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 | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) Index: redhat-linux/fs/dax.c =================================================================== --- redhat-linux.orig/fs/dax.c 2021-04-21 11:51:04.716289502 -0400 +++ redhat-linux/fs/dax.c 2021-04-21 11:52:10.298010850 -0400 @@ -144,6 +144,16 @@ struct wait_exceptional_entry_queue { struct exceptional_entry_key key; }; +/** + * enum dax_entry_wake_mode: waitqueue wakeup toggle + * @WAKE_NEXT: wake only the first waiter in the waitqueue + * @WAKE_ALL: wake all waiters in the waitqueue + */ +enum dax_entry_wake_mode { + WAKE_NEXT, + WAKE_ALL, +}; + static wait_queue_head_t *dax_entry_waitqueue(struct xa_state *xas, void *entry, struct exceptional_entry_key *key) { @@ -182,7 +192,8 @@ static int wake_exceptional_entry_func(w * The important information it's conveying is whether the entry at * this index used to be a PMD entry. */ -static void dax_wake_entry(struct xa_state *xas, void *entry, bool wake_all) +static void dax_wake_entry(struct xa_state *xas, void *entry, + enum dax_entry_wake_mode mode) { struct exceptional_entry_key key; wait_queue_head_t *wq; @@ -196,7 +207,7 @@ static void dax_wake_entry(struct xa_sta * must be in the waitqueue and the following check will see them. */ if (waitqueue_active(wq)) - __wake_up(wq, TASK_NORMAL, wake_all ? 0 : 1, &key); + __wake_up(wq, TASK_NORMAL, mode == WAKE_ALL ? 0 : 1, &key); } /* @@ -268,7 +279,7 @@ static void put_unlocked_entry(struct xa { /* If we were the only waiter woken, wake the next one */ if (entry && !dax_is_conflict(entry)) - dax_wake_entry(xas, entry, false); + dax_wake_entry(xas, entry, WAKE_NEXT); } /* @@ -286,7 +297,7 @@ static void dax_unlock_entry(struct xa_s old = xas_store(xas, entry); xas_unlock_irq(xas); BUG_ON(!dax_is_locked(old)); - dax_wake_entry(xas, entry, false); + dax_wake_entry(xas, entry, WAKE_NEXT); } /* @@ -524,7 +535,7 @@ retry: dax_disassociate_entry(entry, mapping, false); xas_store(xas, NULL); /* undo the PMD join */ - dax_wake_entry(xas, entry, true); + dax_wake_entry(xas, entry, WAKE_ALL); mapping->nrexceptional--; entry = NULL; xas_set(xas, index); @@ -937,7 +948,7 @@ static int dax_writeback_one(struct xa_s xas_lock_irq(xas); xas_store(xas, entry); xas_clear_mark(xas, PAGECACHE_TAG_DIRTY); - dax_wake_entry(xas, entry, false); + dax_wake_entry(xas, entry, WAKE_NEXT); trace_dax_writeback_one(mapping->host, index, count); return ret;
On Wed, Apr 21, 2021 at 11:56:31AM -0400, Vivek Goyal wrote: > +/** > + * enum dax_entry_wake_mode: waitqueue wakeup toggle s/toggle/behaviour/ ? > + * @WAKE_NEXT: wake only the first waiter in the waitqueue > + * @WAKE_ALL: wake all waiters in the waitqueue > + */ > +enum dax_entry_wake_mode { > + WAKE_NEXT, > + WAKE_ALL, > +}; > + > static wait_queue_head_t *dax_entry_waitqueue(struct xa_state *xas, > void *entry, struct exceptional_entry_key *key) > { > @@ -182,7 +192,8 @@ static int wake_exceptional_entry_func(w > * The important information it's conveying is whether the entry at > * this index used to be a PMD entry. > */ > -static void dax_wake_entry(struct xa_state *xas, void *entry, bool wake_all) > +static void dax_wake_entry(struct xa_state *xas, void *entry, > + enum dax_entry_wake_mode mode) It's an awfully verbose name. 'dax_wake_mode'?
On Wed, Apr 21, 2021 at 05:16:24PM +0100, Matthew Wilcox wrote: > On Wed, Apr 21, 2021 at 11:56:31AM -0400, Vivek Goyal wrote: > > +/** > > + * enum dax_entry_wake_mode: waitqueue wakeup toggle > > s/toggle/behaviour/ ? Will do. > > > + * @WAKE_NEXT: wake only the first waiter in the waitqueue > > + * @WAKE_ALL: wake all waiters in the waitqueue > > + */ > > +enum dax_entry_wake_mode { > > + WAKE_NEXT, > > + WAKE_ALL, > > +}; > > + > > static wait_queue_head_t *dax_entry_waitqueue(struct xa_state *xas, > > void *entry, struct exceptional_entry_key *key) > > { > > @@ -182,7 +192,8 @@ static int wake_exceptional_entry_func(w > > * The important information it's conveying is whether the entry at > > * this index used to be a PMD entry. > > */ > > -static void dax_wake_entry(struct xa_state *xas, void *entry, bool wake_all) > > +static void dax_wake_entry(struct xa_state *xas, void *entry, > > + enum dax_entry_wake_mode mode) > > It's an awfully verbose name. 'dax_wake_mode'? Sure. Will change. Vivek >
diff --git a/fs/dax.c b/fs/dax.c index b3d27fdc6775..00978d0838b1 100644 --- a/fs/dax.c +++ b/fs/dax.c @@ -144,6 +144,16 @@ struct wait_exceptional_entry_queue { struct exceptional_entry_key key; }; +/** + * enum dax_entry_wake_mode: waitqueue wakeup toggle + * @WAKE_NEXT: entry was not mutated + * @WAKE_ALL: entry was invalidated, or resized + */ +enum dax_entry_wake_mode { + WAKE_NEXT, + WAKE_ALL, +}; + static wait_queue_head_t *dax_entry_waitqueue(struct xa_state *xas, void *entry, struct exceptional_entry_key *key) { @@ -182,7 +192,8 @@ static int wake_exceptional_entry_func(wait_queue_entry_t *wait, * The important information it's conveying is whether the entry at * this index used to be a PMD entry. */ -static void dax_wake_entry(struct xa_state *xas, void *entry, bool wake_all) +static void dax_wake_entry(struct xa_state *xas, void *entry, + enum dax_entry_wake_mode mode) { struct exceptional_entry_key key; wait_queue_head_t *wq; @@ -196,7 +207,7 @@ static void dax_wake_entry(struct xa_state *xas, void *entry, bool wake_all) * must be in the waitqueue and the following check will see them. */ if (waitqueue_active(wq)) - __wake_up(wq, TASK_NORMAL, wake_all ? 0 : 1, &key); + __wake_up(wq, TASK_NORMAL, mode == WAKE_ALL ? 0 : 1, &key); } /* @@ -268,7 +279,7 @@ static void put_unlocked_entry(struct xa_state *xas, void *entry) { /* If we were the only waiter woken, wake the next one */ if (entry && !dax_is_conflict(entry)) - dax_wake_entry(xas, entry, false); + dax_wake_entry(xas, entry, WAKE_NEXT); } /* @@ -286,7 +297,7 @@ static void dax_unlock_entry(struct xa_state *xas, void *entry) old = xas_store(xas, entry); xas_unlock_irq(xas); BUG_ON(!dax_is_locked(old)); - dax_wake_entry(xas, entry, false); + dax_wake_entry(xas, entry, WAKE_NEXT); } /* @@ -524,7 +535,7 @@ static void *grab_mapping_entry(struct xa_state *xas, dax_disassociate_entry(entry, mapping, false); xas_store(xas, NULL); /* undo the PMD join */ - dax_wake_entry(xas, entry, true); + dax_wake_entry(xas, entry, WAKE_ALL); mapping->nrexceptional--; entry = NULL; xas_set(xas, index); @@ -937,7 +948,7 @@ static int dax_writeback_one(struct xa_state *xas, struct dax_device *dax_dev, xas_lock_irq(xas); xas_store(xas, entry); xas_clear_mark(xas, PAGECACHE_TAG_DIRTY); - dax_wake_entry(xas, entry, false); + dax_wake_entry(xas, entry, WAKE_NEXT); trace_dax_writeback_one(mapping->host, index, count); return ret;
Dan mentioned that he is not very fond of passing around a boolean true/false to specify if only next waiter should be woken up or all waiters should be woken up. He instead prefers that we introduce an enum and make it very explicity at the callsite itself. Easier to read code. This patch should 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 | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-)