2020-06-30 12:38:37

by Michal Suchánek

Subject: [PATCH] dm writecache: reject asynchronous pmem.

The writecache driver does not handle asynchronous pmem. Reject it when
supplied as cache.

Link: https://lore.kernel.org/linux-nvdimm/[email protected]/
Fixes: 6e84200c0a29 ("virtio-pmem: Add virtio pmem driver")

Signed-off-by: Michal Suchanek <[email protected]>
---
drivers/md/dm-writecache.c | 6 ++++++
1 file changed, 6 insertions(+)

diff --git a/drivers/md/dm-writecache.c b/drivers/md/dm-writecache.c
index 30505d70f423..57b0a972f6fd 100644
--- a/drivers/md/dm-writecache.c
+++ b/drivers/md/dm-writecache.c
@@ -2277,6 +2277,12 @@ static int writecache_ctr(struct dm_target *ti, unsigned argc, char **argv)

wc->memory_map_size -= (uint64_t)wc->start_sector << SECTOR_SHIFT;

+ if (!dax_synchronous(wc->ssd_dev->dax_dev)) {
+ r = -EOPNOTSUPP;
+ ti->error = "Asynchronous persistent memory not supported as pmem cache";
+ goto bad;
+ }
+
bio_list_init(&wc->flush_list);
wc->flush_thread = kthread_create(writecache_flush_thread, wc, "dm_writecache_flush");
if (IS_ERR(wc->flush_thread)) {
--
2.26.2

2020-06-30 13:52:04

by Mikulas Patocka

Subject: Re: [PATCH] dm writecache: reject asynchronous pmem.

On Tue, 30 Jun 2020, Michal Suchanek wrote:

> The writecache driver does not handle asynchronous pmem. Reject it when
> supplied as cache.
>
> Link: https://lore.kernel.org/linux-nvdimm/[email protected]/
> Fixes: 6e84200c0a29 ("virtio-pmem: Add virtio pmem driver")
>
> Signed-off-by: Michal Suchanek <[email protected]>
> ---
> drivers/md/dm-writecache.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/md/dm-writecache.c b/drivers/md/dm-writecache.c
> index 30505d70f423..57b0a972f6fd 100644
> --- a/drivers/md/dm-writecache.c
> +++ b/drivers/md/dm-writecache.c
> @@ -2277,6 +2277,12 @@ static int writecache_ctr(struct dm_target *ti, unsigned argc, char **argv)
>
> wc->memory_map_size -= (uint64_t)wc->start_sector << SECTOR_SHIFT;
>
> + if (!dax_synchronous(wc->ssd_dev->dax_dev)) {
> + r = -EOPNOTSUPP;
> + ti->error = "Asynchronous persistent memory not supported as pmem cache";
> + goto bad;
> + }
> +
> bio_list_init(&wc->flush_list);
> wc->flush_thread = kthread_create(writecache_flush_thread, wc, "dm_writecache_flush");
> if (IS_ERR(wc->flush_thread)) {
> --

Hi

Shouldn't this be in the "if (WC_MODE_PMEM(wc))" block?

WC_MODE_PMEM(wc) retrurns true if we are using persistent memory as a
cache device, otherwise we are using generic block device as a cache
device.

Mikulas

2020-06-30 14:14:16

by Michal Suchánek

Subject: Re: [PATCH] dm writecache: reject asynchronous pmem.

On Tue, Jun 30, 2020 at 09:32:01AM -0400, Mikulas Patocka wrote:
>
>
> On Tue, 30 Jun 2020, Michal Suchanek wrote:
>
> > The writecache driver does not handle asynchronous pmem. Reject it when
> > supplied as cache.
> >
> > Link: https://lore.kernel.org/linux-nvdimm/[email protected]/
> > Fixes: 6e84200c0a29 ("virtio-pmem: Add virtio pmem driver")
> >
> > Signed-off-by: Michal Suchanek <[email protected]>
> > ---
> > drivers/md/dm-writecache.c | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/md/dm-writecache.c b/drivers/md/dm-writecache.c
> > index 30505d70f423..57b0a972f6fd 100644
> > --- a/drivers/md/dm-writecache.c
> > +++ b/drivers/md/dm-writecache.c
> > @@ -2277,6 +2277,12 @@ static int writecache_ctr(struct dm_target *ti, unsigned argc, char **argv)
> >
> > wc->memory_map_size -= (uint64_t)wc->start_sector << SECTOR_SHIFT;
> >
> > + if (!dax_synchronous(wc->ssd_dev->dax_dev)) {
> > + r = -EOPNOTSUPP;
> > + ti->error = "Asynchronous persistent memory not supported as pmem cache";
> > + goto bad;
> > + }
> > +
> > bio_list_init(&wc->flush_list);
> > wc->flush_thread = kthread_create(writecache_flush_thread, wc, "dm_writecache_flush");
> > if (IS_ERR(wc->flush_thread)) {
> > --
>
> Hi
>
> Shouldn't this be in the "if (WC_MODE_PMEM(wc))" block?
That should be always the case at this point.
>
> WC_MODE_PMEM(wc) retrurns true if we are using persistent memory as a
> cache device, otherwise we are using generic block device as a cache
> device.
This is to prevent the situation where we have WC_MODE_PMEM(wc) but
cannot guarantee consistency because the async flush is not handled.

Thanks

Michal

2020-06-30 14:40:00

by Mike Snitzer

Subject: Re: dm writecache: reject asynchronous pmem.

On Tue, Jun 30 2020 at 10:10am -0400,
Michal Such?nek <[email protected]> wrote:

> On Tue, Jun 30, 2020 at 09:32:01AM -0400, Mikulas Patocka wrote:
> >
> >
> > On Tue, 30 Jun 2020, Michal Suchanek wrote:
> >
> > > The writecache driver does not handle asynchronous pmem. Reject it when
> > > supplied as cache.
> > >
> > > Link: https://lore.kernel.org/linux-nvdimm/[email protected]/
> > > Fixes: 6e84200c0a29 ("virtio-pmem: Add virtio pmem driver")
> > >
> > > Signed-off-by: Michal Suchanek <[email protected]>
> > > ---
> > > drivers/md/dm-writecache.c | 6 ++++++
> > > 1 file changed, 6 insertions(+)
> > >
> > > diff --git a/drivers/md/dm-writecache.c b/drivers/md/dm-writecache.c
> > > index 30505d70f423..57b0a972f6fd 100644
> > > --- a/drivers/md/dm-writecache.c
> > > +++ b/drivers/md/dm-writecache.c
> > > @@ -2277,6 +2277,12 @@ static int writecache_ctr(struct dm_target *ti, unsigned argc, char **argv)
> > >
> > > wc->memory_map_size -= (uint64_t)wc->start_sector << SECTOR_SHIFT;
> > >
> > > + if (!dax_synchronous(wc->ssd_dev->dax_dev)) {
> > > + r = -EOPNOTSUPP;
> > > + ti->error = "Asynchronous persistent memory not supported as pmem cache";
> > > + goto bad;
> > > + }
> > > +
> > > bio_list_init(&wc->flush_list);
> > > wc->flush_thread = kthread_create(writecache_flush_thread, wc, "dm_writecache_flush");
> > > if (IS_ERR(wc->flush_thread)) {
> > > --
> >
> > Hi
> >
> > Shouldn't this be in the "if (WC_MODE_PMEM(wc))" block?
> That should be always the case at this point.
> >
> > WC_MODE_PMEM(wc) retrurns true if we are using persistent memory as a
> > cache device, otherwise we are using generic block device as a cache
> > device.
>
> This is to prevent the situation where we have WC_MODE_PMEM(wc) but
> cannot guarantee consistency because the async flush is not handled.

The writecache operates in 2 modes. SSD or PMEM. Mikulas is saying
your dax_synchronous() check should go within a WC_MODE_PMEM(wc) block
because it doesn't make sense to do the check when in SSD mode.

Mike

2020-06-30 14:44:32

by Michal Suchánek

Subject: Re: dm writecache: reject asynchronous pmem.

On Tue, Jun 30, 2020 at 09:36:33AM -0400, Mike Snitzer wrote:
> On Tue, Jun 30 2020 at 10:10am -0400,
> Michal Such?nek <[email protected]> wrote:
>
> > On Tue, Jun 30, 2020 at 09:32:01AM -0400, Mikulas Patocka wrote:
> > >
> > >
> > > On Tue, 30 Jun 2020, Michal Suchanek wrote:
> > >
> > > > The writecache driver does not handle asynchronous pmem. Reject it when
> > > > supplied as cache.
> > > >
> > > > Link: https://lore.kernel.org/linux-nvdimm/[email protected]/
> > > > Fixes: 6e84200c0a29 ("virtio-pmem: Add virtio pmem driver")
> > > >
> > > > Signed-off-by: Michal Suchanek <[email protected]>
> > > > ---
> > > > drivers/md/dm-writecache.c | 6 ++++++
> > > > 1 file changed, 6 insertions(+)
> > > >
> > > > diff --git a/drivers/md/dm-writecache.c b/drivers/md/dm-writecache.c
> > > > index 30505d70f423..57b0a972f6fd 100644
> > > > --- a/drivers/md/dm-writecache.c
> > > > +++ b/drivers/md/dm-writecache.c
> > > > @@ -2277,6 +2277,12 @@ static int writecache_ctr(struct dm_target *ti, unsigned argc, char **argv)
> > > >
> > > > wc->memory_map_size -= (uint64_t)wc->start_sector << SECTOR_SHIFT;
> > > >
> > > > + if (!dax_synchronous(wc->ssd_dev->dax_dev)) {
> > > > + r = -EOPNOTSUPP;
> > > > + ti->error = "Asynchronous persistent memory not supported as pmem cache";
> > > > + goto bad;
> > > > + }
> > > > +
> > > > bio_list_init(&wc->flush_list);
> > > > wc->flush_thread = kthread_create(writecache_flush_thread, wc, "dm_writecache_flush");
> > > > if (IS_ERR(wc->flush_thread)) {
> > > > --
> > >
> > > Hi
> > >
> > > Shouldn't this be in the "if (WC_MODE_PMEM(wc))" block?
> > That should be always the case at this point.
> > >
> > > WC_MODE_PMEM(wc) retrurns true if we are using persistent memory as a
> > > cache device, otherwise we are using generic block device as a cache
> > > device.
> >
> > This is to prevent the situation where we have WC_MODE_PMEM(wc) but
> > cannot guarantee consistency because the async flush is not handled.
>
> The writecache operates in 2 modes. SSD or PMEM. Mikulas is saying
> your dax_synchronous() check should go within a WC_MODE_PMEM(wc) block
> because it doesn't make sense to do the check when in SSD mode.

Indeed, it's in the wrong if/else branch.

Thanks

Michal

2020-06-30 14:55:34

by Michal Suchánek

Subject: [PATCH v2] dm writecache: reject asynchronous pmem.

The writecache driver does not handle asynchronous pmem. Reject it when
supplied as cache.

Link: https://lore.kernel.org/linux-nvdimm/[email protected]/
Fixes: 6e84200c0a29 ("virtio-pmem: Add virtio pmem driver")

Signed-off-by: Michal Suchanek <[email protected]>
---
drivers/md/dm-writecache.c | 6 ++++++
1 file changed, 6 insertions(+)

diff --git a/drivers/md/dm-writecache.c b/drivers/md/dm-writecache.c
index 30505d70f423..1e4f37249e28 100644
--- a/drivers/md/dm-writecache.c
+++ b/drivers/md/dm-writecache.c
@@ -2271,6 +2271,12 @@ static int writecache_ctr(struct dm_target *ti, unsigned argc, char **argv)
ti->error = "Unable to map persistent memory for cache";
goto bad;
}
+
+ if (!dax_synchronous(wc->ssd_dev->dax_dev)) {
+ r = -EOPNOTSUPP;
+ ti->error = "Asynchronous persistent memory not supported as pmem cache";
+ goto bad;
+ }
} else {
size_t n_blocks, n_metadata_blocks;
uint64_t n_bitmap_bits;
--
2.26.2

2020-06-30 15:03:43

by Mikulas Patocka

Subject: Re: [PATCH v2] dm writecache: reject asynchronous pmem.

On Tue, 30 Jun 2020, Michal Suchanek wrote:

> The writecache driver does not handle asynchronous pmem. Reject it when
> supplied as cache.
>
> Link: https://lore.kernel.org/linux-nvdimm/[email protected]/
> Fixes: 6e84200c0a29 ("virtio-pmem: Add virtio pmem driver")
>
> Signed-off-by: Michal Suchanek <[email protected]>

OK. I suggest to move this test before persistent_memory_claim (so that
you don't try to map the whole device).

Mikulas

> ---
> drivers/md/dm-writecache.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/md/dm-writecache.c b/drivers/md/dm-writecache.c
> index 30505d70f423..1e4f37249e28 100644
> --- a/drivers/md/dm-writecache.c
> +++ b/drivers/md/dm-writecache.c
> @@ -2271,6 +2271,12 @@ static int writecache_ctr(struct dm_target *ti, unsigned argc, char **argv)
> ti->error = "Unable to map persistent memory for cache";
> goto bad;
> }
> +
> + if (!dax_synchronous(wc->ssd_dev->dax_dev)) {
> + r = -EOPNOTSUPP;
> + ti->error = "Asynchronous persistent memory not supported as pmem cache";
> + goto bad;
> + }
> } else {
> size_t n_blocks, n_metadata_blocks;
> uint64_t n_bitmap_bits;
> --
> 2.26.2
>

2020-06-30 15:50:53

by Michal Suchánek

Subject: [PATCH v3] dm writecache: reject asynchronous pmem.

The writecache driver does not handle asynchronous pmem. Reject it when
supplied as cache.

Link: https://lore.kernel.org/linux-nvdimm/[email protected]/
Fixes: 6e84200c0a29 ("virtio-pmem: Add virtio pmem driver")

Signed-off-by: Michal Suchanek <[email protected]>
---
drivers/md/dm-writecache.c | 6 ++++++
1 file changed, 6 insertions(+)

diff --git a/drivers/md/dm-writecache.c b/drivers/md/dm-writecache.c
index 30505d70f423..5358894bb9fd 100644
--- a/drivers/md/dm-writecache.c
+++ b/drivers/md/dm-writecache.c
@@ -2266,6 +2266,12 @@ static int writecache_ctr(struct dm_target *ti, unsigned argc, char **argv)
}

if (WC_MODE_PMEM(wc)) {
+ if (!dax_synchronous(wc->ssd_dev->dax_dev)) {
+ r = -EOPNOTSUPP;
+ ti->error = "Asynchronous persistent memory not supported as pmem cache";
+ goto bad;
+ }
+
r = persistent_memory_claim(wc);
if (r) {
ti->error = "Unable to map persistent memory for cache";
--
2.26.2

2020-06-30 16:13:13

by Mikulas Patocka

Subject: Re: [PATCH v3] dm writecache: reject asynchronous pmem.

On Tue, 30 Jun 2020, Michal Suchanek wrote:

> The writecache driver does not handle asynchronous pmem. Reject it when
> supplied as cache.
>
> Link: https://lore.kernel.org/linux-nvdimm/[email protected]/
> Fixes: 6e84200c0a29 ("virtio-pmem: Add virtio pmem driver")
>
> Signed-off-by: Michal Suchanek <[email protected]>

Acked-by: Mikulas Patocka <[email protected]>

> ---
> drivers/md/dm-writecache.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/drivers/md/dm-writecache.c b/drivers/md/dm-writecache.c
> index 30505d70f423..5358894bb9fd 100644
> --- a/drivers/md/dm-writecache.c
> +++ b/drivers/md/dm-writecache.c
> @@ -2266,6 +2266,12 @@ static int writecache_ctr(struct dm_target *ti, unsigned argc, char **argv)
> }
>
> if (WC_MODE_PMEM(wc)) {
> + if (!dax_synchronous(wc->ssd_dev->dax_dev)) {
> + r = -EOPNOTSUPP;
> + ti->error = "Asynchronous persistent memory not supported as pmem cache";
> + goto bad;
> + }
> +
> r = persistent_memory_claim(wc);
> if (r) {
> ti->error = "Unable to map persistent memory for cache";
> --
> 2.26.2
>

2020-06-30 16:16:59

by Mikulas Patocka

Subject: Re: [PATCH v3] dm writecache: reject asynchronous pmem.

On Tue, 30 Jun 2020, Mikulas Patocka wrote:

>
>
> On Tue, 30 Jun 2020, Michal Suchanek wrote:
>
> > The writecache driver does not handle asynchronous pmem. Reject it when
> > supplied as cache.
> >
> > Link: https://lore.kernel.org/linux-nvdimm/[email protected]/
> > Fixes: 6e84200c0a29 ("virtio-pmem: Add virtio pmem driver")
> >
> > Signed-off-by: Michal Suchanek <[email protected]>
>
> Acked-by: Mikulas Patocka <[email protected]>

BTW, we should also add

Cc: [email protected] # v5.3+

> > ---
> > drivers/md/dm-writecache.c | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/md/dm-writecache.c b/drivers/md/dm-writecache.c
> > index 30505d70f423..5358894bb9fd 100644
> > --- a/drivers/md/dm-writecache.c
> > +++ b/drivers/md/dm-writecache.c
> > @@ -2266,6 +2266,12 @@ static int writecache_ctr(struct dm_target *ti, unsigned argc, char **argv)
> > }
> >
> > if (WC_MODE_PMEM(wc)) {
> > + if (!dax_synchronous(wc->ssd_dev->dax_dev)) {
> > + r = -EOPNOTSUPP;
> > + ti->error = "Asynchronous persistent memory not supported as pmem cache";
> > + goto bad;
> > + }
> > +
> > r = persistent_memory_claim(wc);
> > if (r) {
> > ti->error = "Unable to map persistent memory for cache";
> > --
> > 2.26.2
> >
>