Hi,
This is V4 of the patches. Posted V3 here.
https://lore.kernel.org/linux-fsdevel/[email protected]/
Changes since V3 are.
- Renamed "enum dax_entry_wake_mode" to "enum dax_wake_mode" (Matthew Wilcox)
- Changed description of WAKE_NEXT and WAKE_ALL (Jan Kara)
- Got rid of a comment (Greg Kurz)
Thanks
Vivek
Vivek Goyal (3):
dax: Add an enum for specifying dax wakup mode
dax: Add a wakeup mode parameter to put_unlocked_entry()
dax: Wake up all waiters after invalidating dax entry
fs/dax.c | 35 +++++++++++++++++++++++------------
1 file changed, 23 insertions(+), 12 deletions(-)
--
2.25.4
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.
Reviewed-by: Greg Kurz <[email protected]>
Reviewed-by: Jan Kara <[email protected]>
Suggested-by: Dan Williams <[email protected]>
Signed-off-by: Vivek Goyal <[email protected]>
---
fs/dax.c | 23 +++++++++++++++++------
1 file changed, 17 insertions(+), 6 deletions(-)
diff --git a/fs/dax.c b/fs/dax.c
index b3d27fdc6775..4b1918b9ad97 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_wake_mode: waitqueue wakeup behaviour
+ * @WAKE_NEXT: wake only the first waiter in the waitqueue
+ * @WAKE_ALL: wake all waiters in the waitqueue
+ */
+enum dax_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_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 Fri, Apr 23, 2021 at 6:07 AM Vivek Goyal <[email protected]> wrote:
>
> Hi,
>
> This is V4 of the patches. Posted V3 here.
>
> https://lore.kernel.org/linux-fsdevel/[email protected]/
>
> Changes since V3 are.
>
> - Renamed "enum dax_entry_wake_mode" to "enum dax_wake_mode" (Matthew Wilcox)
> - Changed description of WAKE_NEXT and WAKE_ALL (Jan Kara)
> - Got rid of a comment (Greg Kurz)
Looks good Vivek, thanks for the resend.
On Fri, Apr 23, 2021 at 09:07:21AM -0400, Vivek Goyal wrote:
> +enum dax_wake_mode {
> + WAKE_NEXT,
> + WAKE_ALL,
> +};
Why define them in this order when ...
> @@ -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);
... they're used like this? This is almost as bad as
enum bool {
true,
false,
};
On Mon, Apr 26, 2021 at 02:46:32PM +0100, Matthew Wilcox wrote:
> On Fri, Apr 23, 2021 at 09:07:21AM -0400, Vivek Goyal wrote:
> > +enum dax_wake_mode {
> > + WAKE_NEXT,
> > + WAKE_ALL,
> > +};
>
> Why define them in this order when ...
>
> > @@ -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);
>
> ... they're used like this? This is almost as bad as
>
> enum bool {
> true,
> false,
> };
Hi Matthew,
So you prefer that I should switch order of WAKE_NEXT and WAKE_ALL?
enum dax_wake_mode {
WAKE_ALL,
WAKE_NEXT,
};
And then do following to wake task.
if (waitqueue_active(wq))
__wake_up(wq, TASK_NORMAL, mode, &key);
I am fine with this if you like this better.
Or you are suggesting that don't introduce "enum dax_wake_mode" to
begin with.
Vivek
On Mon, Apr 26, 2021 at 01:52:17PM -0400, Vivek Goyal wrote:
> On Mon, Apr 26, 2021 at 02:46:32PM +0100, Matthew Wilcox wrote:
> > On Fri, Apr 23, 2021 at 09:07:21AM -0400, Vivek Goyal wrote:
> > > +enum dax_wake_mode {
> > > + WAKE_NEXT,
> > > + WAKE_ALL,
> > > +};
> >
> > Why define them in this order when ...
> >
> > > @@ -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);
> >
> > ... they're used like this? This is almost as bad as
> >
> > enum bool {
> > true,
> > false,
> > };
>
> Hi Matthew,
>
> So you prefer that I should switch order of WAKE_NEXT and WAKE_ALL?
>
> enum dax_wake_mode {
> WAKE_ALL,
> WAKE_NEXT,
> };
That, yes.
> And then do following to wake task.
>
> if (waitqueue_active(wq))
> __wake_up(wq, TASK_NORMAL, mode, &key);
No, the third argument to __wake_up() is a count, not an enum. It just so
happens that '0' means 'all' and we only ever wake up 1 and not, say, 5.
So the logical way to define the enum is ALL, NEXT which _just happens
to match_ the usage of __wake_up().
On Mon, Apr 26, 2021 at 07:02:11PM +0100, Matthew Wilcox wrote:
> On Mon, Apr 26, 2021 at 01:52:17PM -0400, Vivek Goyal wrote:
> > On Mon, Apr 26, 2021 at 02:46:32PM +0100, Matthew Wilcox wrote:
> > > On Fri, Apr 23, 2021 at 09:07:21AM -0400, Vivek Goyal wrote:
> > > > +enum dax_wake_mode {
> > > > + WAKE_NEXT,
> > > > + WAKE_ALL,
> > > > +};
> > >
> > > Why define them in this order when ...
> > >
> > > > @@ -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);
> > >
> > > ... they're used like this? This is almost as bad as
> > >
> > > enum bool {
> > > true,
> > > false,
> > > };
> >
> > Hi Matthew,
> >
> > So you prefer that I should switch order of WAKE_NEXT and WAKE_ALL?
> >
> > enum dax_wake_mode {
> > WAKE_ALL,
> > WAKE_NEXT,
> > };
>
> That, yes.
>
> > And then do following to wake task.
> >
> > if (waitqueue_active(wq))
> > __wake_up(wq, TASK_NORMAL, mode, &key);
>
> No, the third argument to __wake_up() is a count, not an enum. It just so
> happens that '0' means 'all' and we only ever wake up 1 and not, say, 5.
> So the logical way to define the enum is ALL, NEXT which _just happens
> to match_ the usage of __wake_up().
Ok, In that case, I will retain existing code.
__wake_up(wq, TASK_NORMAL, mode == WAKE_ALL ? 0 : 1, &key);
Vivek