[dm-devel] [PATCH] dm-integrity: Fix flush with external metadata device

[dm-devel] [PATCH] dm-integrity: Fix flush with external metadata device

[Lukas Straub]

[-- Attachment #1.1: Type: text/plain, Size: 1481 bytes --]

With an external metadata device, flush requests aren't passed down
to the data device.

Fix this by issuing flush in the right places: In integrity_commit
when not in journal mode, in do_journal_write after writing the
contents of the journal to the disk and in dm_integrity_postsuspend.

Signed-off-by: Lukas Straub <lukasstraub2@web.de>
---
 drivers/md/dm-integrity.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/md/dm-integrity.c b/drivers/md/dm-integrity.c
index 5a7a1b90e671..a26ed65869f6 100644
--- a/drivers/md/dm-integrity.c
+++ b/drivers/md/dm-integrity.c
@@ -2196,6 +2196,8 @@ static void integrity_commit(struct work_struct *w)
 	if (unlikely(ic->mode != 'J')) {
 		spin_unlock_irq(&ic->endio_wait.lock);
 		dm_integrity_flush_buffers(ic);
+		if (ic->meta_dev)
+			blkdev_issue_flush(ic->dev->bdev, GFP_NOIO);
 		goto release_flush_bios;
 	}
 
@@ -2410,6 +2412,9 @@ static void do_journal_write(struct dm_integrity_c *ic, unsigned write_start,
 	wait_for_completion_io(&comp.comp);
 
 	dm_integrity_flush_buffers(ic);
+	if (ic->meta_dev)
+		blkdev_issue_flush(ic->dev->bdev, GFP_NOIO);
+
 }
 
 static void integrity_writer(struct work_struct *w)
@@ -2949,6 +2954,9 @@ static void dm_integrity_postsuspend(struct dm_target *ti)
 #endif
 	}
 
+	if (ic->meta_dev)
+		blkdev_issue_flush(ic->dev->bdev, GFP_NOIO);
+
 	BUG_ON(!RB_EMPTY_ROOT(&ic->in_progress));
 
 	ic->journal_uptodate = true;
-- 
2.20.1

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 93 bytes --]

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH] dm-integrity: Fix flush with external metadata device

[Lukas Straub]

[-- Attachment #1.1: Type: text/plain, Size: 1659 bytes --]

On Sun, 20 Dec 2020 14:02:22 +0100
Lukas Straub <lukasstraub2@web.de> wrote:

> With an external metadata device, flush requests aren't passed down
> to the data device.
> 
> Fix this by issuing flush in the right places: In integrity_commit
> when not in journal mode, in do_journal_write after writing the
> contents of the journal to the disk and in dm_integrity_postsuspend.
> 
> Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> ---
>  drivers/md/dm-integrity.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/md/dm-integrity.c b/drivers/md/dm-integrity.c
> index 5a7a1b90e671..a26ed65869f6 100644
> --- a/drivers/md/dm-integrity.c
> +++ b/drivers/md/dm-integrity.c
> @@ -2196,6 +2196,8 @@ static void integrity_commit(struct work_struct *w)
>  	if (unlikely(ic->mode != 'J')) {
>  		spin_unlock_irq(&ic->endio_wait.lock);
>  		dm_integrity_flush_buffers(ic);
> +		if (ic->meta_dev)
> +			blkdev_issue_flush(ic->dev->bdev, GFP_NOIO);
>  		goto release_flush_bios;
>  	}
>  
> @@ -2410,6 +2412,9 @@ static void do_journal_write(struct dm_integrity_c *ic, unsigned write_start,
>  	wait_for_completion_io(&comp.comp);
>  
>  	dm_integrity_flush_buffers(ic);
> +	if (ic->meta_dev)
> +		blkdev_issue_flush(ic->dev->bdev, GFP_NOIO);
> +
>  }
>  
>  static void integrity_writer(struct work_struct *w)
> @@ -2949,6 +2954,9 @@ static void dm_integrity_postsuspend(struct dm_target *ti)
>  #endif
>  	}
>  
> +	if (ic->meta_dev)
> +		blkdev_issue_flush(ic->dev->bdev, GFP_NOIO);
> +
>  	BUG_ON(!RB_EMPTY_ROOT(&ic->in_progress));
>  
>  	ic->journal_uptodate = true;

Ping...

-- 


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 93 bytes --]

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] dm-integrity: Fix flush with external metadata device

[Mike Snitzer]

On Sun, Dec 20 2020 at  8:02am -0500,
Lukas Straub <lukasstraub2@web.de> wrote:

> With an external metadata device, flush requests aren't passed down
> to the data device.
> 
> Fix this by issuing flush in the right places: In integrity_commit
> when not in journal mode, in do_journal_write after writing the
> contents of the journal to the disk and in dm_integrity_postsuspend.
> 
> Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> ---
>  drivers/md/dm-integrity.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/md/dm-integrity.c b/drivers/md/dm-integrity.c
> index 5a7a1b90e671..a26ed65869f6 100644
> --- a/drivers/md/dm-integrity.c
> +++ b/drivers/md/dm-integrity.c
> @@ -2196,6 +2196,8 @@ static void integrity_commit(struct work_struct *w)
>  	if (unlikely(ic->mode != 'J')) {
>  		spin_unlock_irq(&ic->endio_wait.lock);
>  		dm_integrity_flush_buffers(ic);
> +		if (ic->meta_dev)
> +			blkdev_issue_flush(ic->dev->bdev, GFP_NOIO);
>  		goto release_flush_bios;
>  	}
>  
> @@ -2410,6 +2412,9 @@ static void do_journal_write(struct dm_integrity_c *ic, unsigned write_start,
>  	wait_for_completion_io(&comp.comp);
>  
>  	dm_integrity_flush_buffers(ic);
> +	if (ic->meta_dev)
> +		blkdev_issue_flush(ic->dev->bdev, GFP_NOIO);
> +
>  }
>  
>  static void integrity_writer(struct work_struct *w)
> @@ -2949,6 +2954,9 @@ static void dm_integrity_postsuspend(struct dm_target *ti)
>  #endif
>  	}
>  
> +	if (ic->meta_dev)
> +		blkdev_issue_flush(ic->dev->bdev, GFP_NOIO);
> +
>  	BUG_ON(!RB_EMPTY_ROOT(&ic->in_progress));
>  
>  	ic->journal_uptodate = true;
> -- 
> 2.20.1


Seems like a pretty bad oversight... but shouldn't you also make sure to
flush the data device _before_ the metadata is flushed?

Mike

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] dm-integrity: Fix flush with external metadata device

[Mikulas Patocka]

On Mon, 4 Jan 2021, Mike Snitzer wrote:

> On Sun, Dec 20 2020 at  8:02am -0500,
> Lukas Straub <lukasstraub2@web.de> wrote:
> 
> > With an external metadata device, flush requests aren't passed down
> > to the data device.
> > 
> > Fix this by issuing flush in the right places: In integrity_commit
> > when not in journal mode, in do_journal_write after writing the
> > contents of the journal to the disk and in dm_integrity_postsuspend.
> > 
> > Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> > ---
> >  drivers/md/dm-integrity.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/md/dm-integrity.c b/drivers/md/dm-integrity.c
> > index 5a7a1b90e671..a26ed65869f6 100644
> > --- a/drivers/md/dm-integrity.c
> > +++ b/drivers/md/dm-integrity.c
> > @@ -2196,6 +2196,8 @@ static void integrity_commit(struct work_struct *w)
> >  	if (unlikely(ic->mode != 'J')) {
> >  		spin_unlock_irq(&ic->endio_wait.lock);
> >  		dm_integrity_flush_buffers(ic);
> > +		if (ic->meta_dev)
> > +			blkdev_issue_flush(ic->dev->bdev, GFP_NOIO);
> >  		goto release_flush_bios;
> >  	}
> >  
> > @@ -2410,6 +2412,9 @@ static void do_journal_write(struct dm_integrity_c *ic, unsigned write_start,
> >  	wait_for_completion_io(&comp.comp);
> >  
> >  	dm_integrity_flush_buffers(ic);
> > +	if (ic->meta_dev)
> > +		blkdev_issue_flush(ic->dev->bdev, GFP_NOIO);
> > +
> >  }
> >  
> >  static void integrity_writer(struct work_struct *w)
> > @@ -2949,6 +2954,9 @@ static void dm_integrity_postsuspend(struct dm_target *ti)
> >  #endif
> >  	}
> >  
> > +	if (ic->meta_dev)
> > +		blkdev_issue_flush(ic->dev->bdev, GFP_NOIO);
> > +
> >  	BUG_ON(!RB_EMPTY_ROOT(&ic->in_progress));
> >  
> >  	ic->journal_uptodate = true;
> > -- 
> > 2.20.1
> 
> 
> Seems like a pretty bad oversight... but shouldn't you also make sure to
> flush the data device _before_ the metadata is flushed?
> 
> Mike

I think, ordering is not a problem.

A disk may flush its cache spontaneously anytime, so it doesn't matter in 
which order do we flush them. Similarly a dm-bufio buffer may be flushed 
anytime - if the machine is running out of memory and a dm-bufio shrinker 
is called.

I'll send another patch for this - I've created a patch that flushes the 
metadata device cache and data device cache in parallel, so that 
performance degradation is reduced.

My patch also doesn't use GFP_NOIO allocation - which can in theory 
deadlock if we are swapping on dm-integrity device.

Mikulas

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH] dm-integrity: Fix flush with external metadata device

[Mikulas Patocka]

With external metadata device, flush requests are not passed down to the
data device.

Fix this by submitting the flush request in dm_integrity_flush_buffers. In
order to not degrade performance, we overlap the data device flush with
the metadata device flush.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Reported-by: Lukas Straub <lukasstraub2@web.de>
Cc: stable@vger.kernel.org

---
 drivers/md/dm-bufio.c     |    6 ++++
 drivers/md/dm-integrity.c |   60 +++++++++++++++++++++++++++++++++++++---------
 include/linux/dm-bufio.h  |    1 
 3 files changed, 56 insertions(+), 11 deletions(-)

Index: linux-2.6/drivers/md/dm-integrity.c
===================================================================
--- linux-2.6.orig/drivers/md/dm-integrity.c	2021-01-07 17:22:39.000000000 +0100
+++ linux-2.6/drivers/md/dm-integrity.c	2021-01-08 15:51:19.000000000 +0100
@@ -1379,12 +1379,52 @@ thorough_test:
 #undef MAY_BE_HASH
 }
 
-static void dm_integrity_flush_buffers(struct dm_integrity_c *ic)
+struct flush_request {
+	struct dm_io_request io_req;
+	struct dm_io_region io_reg;
+	struct dm_integrity_c *ic;
+	struct completion comp;
+};
+
+static void flush_notify(unsigned long error, void *fr_)
+{
+	struct flush_request *fr = fr_;
+	if (unlikely(error != 0))
+		dm_integrity_io_error(fr->ic, "flusing disk cache", -EIO);
+	complete(&fr->comp);
+}
+
+static void dm_integrity_flush_buffers(struct dm_integrity_c *ic, bool flush_data)
 {
 	int r;
+
+	struct flush_request fr;
+
+	if (!ic->meta_dev)
+		flush_data = false;
+	if (flush_data) {
+		fr.io_req.bi_op = REQ_OP_WRITE,
+		fr.io_req.bi_op_flags = REQ_PREFLUSH | REQ_SYNC,
+		fr.io_req.mem.type = DM_IO_KMEM,
+		fr.io_req.mem.ptr.addr = NULL,
+		fr.io_req.notify.fn = flush_notify,
+		fr.io_req.notify.context = &fr;
+		fr.io_req.client = dm_bufio_get_dm_io_client(ic->bufio),
+		fr.io_reg.bdev = ic->dev->bdev,
+		fr.io_reg.sector = 0,
+		fr.io_reg.count = 0,
+		fr.ic = ic;
+		init_completion(&fr.comp);
+		r = dm_io(&fr.io_req, 1, &fr.io_reg, NULL);
+		BUG_ON(r);
+	}
+
 	r = dm_bufio_write_dirty_buffers(ic->bufio);
 	if (unlikely(r))
 		dm_integrity_io_error(ic, "writing tags", r);
+
+	if (flush_data)
+		wait_for_completion(&fr.comp);
 }
 
 static void sleep_on_endio_wait(struct dm_integrity_c *ic)
@@ -2110,7 +2150,7 @@ offload_to_thread:
 
 	if (unlikely(dio->op == REQ_OP_DISCARD) && likely(ic->mode != 'D')) {
 		integrity_metadata(&dio->work);
-		dm_integrity_flush_buffers(ic);
+		dm_integrity_flush_buffers(ic, false);
 
 		dio->in_flight = (atomic_t)ATOMIC_INIT(1);
 		dio->completion = NULL;
@@ -2195,7 +2235,7 @@ static void integrity_commit(struct work
 	flushes = bio_list_get(&ic->flush_bio_list);
 	if (unlikely(ic->mode != 'J')) {
 		spin_unlock_irq(&ic->endio_wait.lock);
-		dm_integrity_flush_buffers(ic);
+		dm_integrity_flush_buffers(ic, true);
 		goto release_flush_bios;
 	}
 
@@ -2409,7 +2449,7 @@ skip_io:
 	complete_journal_op(&comp);
 	wait_for_completion_io(&comp.comp);
 
-	dm_integrity_flush_buffers(ic);
+	dm_integrity_flush_buffers(ic, true);
 }
 
 static void integrity_writer(struct work_struct *w)
@@ -2451,7 +2491,7 @@ static void recalc_write_super(struct dm
 {
 	int r;
 
-	dm_integrity_flush_buffers(ic);
+	dm_integrity_flush_buffers(ic, false);
 	if (dm_integrity_failed(ic))
 		return;
 
@@ -2654,7 +2694,7 @@ static void bitmap_flush_work(struct wor
 	unsigned long limit;
 	struct bio *bio;
 
-	dm_integrity_flush_buffers(ic);
+	dm_integrity_flush_buffers(ic, false);
 
 	range.logical_sector = 0;
 	range.n_sectors = ic->provided_data_sectors;
@@ -2663,9 +2703,7 @@ static void bitmap_flush_work(struct wor
 	add_new_range_and_wait(ic, &range);
 	spin_unlock_irq(&ic->endio_wait.lock);
 
-	dm_integrity_flush_buffers(ic);
-	if (ic->meta_dev)
-		blkdev_issue_flush(ic->dev->bdev, GFP_NOIO);
+	dm_integrity_flush_buffers(ic, true);
 
 	limit = ic->provided_data_sectors;
 	if (ic->sb->flags & cpu_to_le32(SB_FLAG_RECALCULATING)) {
@@ -2934,11 +2972,11 @@ static void dm_integrity_postsuspend(str
 		if (ic->meta_dev)
 			queue_work(ic->writer_wq, &ic->writer_work);
 		drain_workqueue(ic->writer_wq);
-		dm_integrity_flush_buffers(ic);
+		dm_integrity_flush_buffers(ic, true);
 	}
 
 	if (ic->mode == 'B') {
-		dm_integrity_flush_buffers(ic);
+		dm_integrity_flush_buffers(ic, true);
 #if 1
 		/* set to 0 to test bitmap replay code */
 		init_journal(ic, 0, ic->journal_sections, 0);
Index: linux-2.6/include/linux/dm-bufio.h
===================================================================
--- linux-2.6.orig/include/linux/dm-bufio.h	2020-09-05 10:01:42.000000000 +0200
+++ linux-2.6/include/linux/dm-bufio.h	2021-01-08 15:12:31.000000000 +0100
@@ -150,6 +150,7 @@ void dm_bufio_set_minimum_buffers(struct
 
 unsigned dm_bufio_get_block_size(struct dm_bufio_client *c);
 sector_t dm_bufio_get_device_size(struct dm_bufio_client *c);
+struct dm_io_client *dm_bufio_get_dm_io_client(struct dm_bufio_client *c);
 sector_t dm_bufio_get_block_number(struct dm_buffer *b);
 void *dm_bufio_get_block_data(struct dm_buffer *b);
 void *dm_bufio_get_aux_data(struct dm_buffer *b);
Index: linux-2.6/drivers/md/dm-bufio.c
===================================================================
--- linux-2.6.orig/drivers/md/dm-bufio.c	2021-01-08 15:11:20.000000000 +0100
+++ linux-2.6/drivers/md/dm-bufio.c	2021-01-08 15:12:25.000000000 +0100
@@ -1534,6 +1534,12 @@ sector_t dm_bufio_get_device_size(struct
 }
 EXPORT_SYMBOL_GPL(dm_bufio_get_device_size);
 
+struct dm_io_client *dm_bufio_get_dm_io_client(struct dm_bufio_client *c)
+{
+	return c->dm_io;
+}
+EXPORT_SYMBOL_GPL(dm_bufio_get_dm_io_client);
+
 sector_t dm_bufio_get_block_number(struct dm_buffer *b)
 {
 	return b->block;

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH] dm-integrity: Fix flush with external metadata device

[Lukas Straub]

[-- Attachment #1.1: Type: text/plain, Size: 6411 bytes --]

On Fri, 8 Jan 2021 11:15:56 -0500 (EST)
Mikulas Patocka <mpatocka@redhat.com> wrote:

> With external metadata device, flush requests are not passed down to the
> data device.
> 
> Fix this by submitting the flush request in dm_integrity_flush_buffers. In
> order to not degrade performance, we overlap the data device flush with
> the metadata device flush.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> Reported-by: Lukas Straub <lukasstraub2@web.de>
> Cc: stable@vger.kernel.org

Looks good to me.
Reviewed-by: Lukas Straub <lukasstraub2@web.de>

Regards,
Lukas Straub

> ---
>  drivers/md/dm-bufio.c     |    6 ++++
>  drivers/md/dm-integrity.c |   60 +++++++++++++++++++++++++++++++++++++---------
>  include/linux/dm-bufio.h  |    1 
>  3 files changed, 56 insertions(+), 11 deletions(-)
> 
> Index: linux-2.6/drivers/md/dm-integrity.c
> ===================================================================
> --- linux-2.6.orig/drivers/md/dm-integrity.c	2021-01-07 17:22:39.000000000 +0100
> +++ linux-2.6/drivers/md/dm-integrity.c	2021-01-08 15:51:19.000000000 +0100
> @@ -1379,12 +1379,52 @@ thorough_test:
>  #undef MAY_BE_HASH
>  }
>  
> -static void dm_integrity_flush_buffers(struct dm_integrity_c *ic)
> +struct flush_request {
> +	struct dm_io_request io_req;
> +	struct dm_io_region io_reg;
> +	struct dm_integrity_c *ic;
> +	struct completion comp;
> +};
> +
> +static void flush_notify(unsigned long error, void *fr_)
> +{
> +	struct flush_request *fr = fr_;
> +	if (unlikely(error != 0))
> +		dm_integrity_io_error(fr->ic, "flusing disk cache", -EIO);
> +	complete(&fr->comp);
> +}
> +
> +static void dm_integrity_flush_buffers(struct dm_integrity_c *ic, bool flush_data)
>  {
>  	int r;
> +
> +	struct flush_request fr;
> +
> +	if (!ic->meta_dev)
> +		flush_data = false;
> +	if (flush_data) {
> +		fr.io_req.bi_op = REQ_OP_WRITE,
> +		fr.io_req.bi_op_flags = REQ_PREFLUSH | REQ_SYNC,
> +		fr.io_req.mem.type = DM_IO_KMEM,
> +		fr.io_req.mem.ptr.addr = NULL,
> +		fr.io_req.notify.fn = flush_notify,
> +		fr.io_req.notify.context = &fr;
> +		fr.io_req.client = dm_bufio_get_dm_io_client(ic->bufio),
> +		fr.io_reg.bdev = ic->dev->bdev,
> +		fr.io_reg.sector = 0,
> +		fr.io_reg.count = 0,
> +		fr.ic = ic;
> +		init_completion(&fr.comp);
> +		r = dm_io(&fr.io_req, 1, &fr.io_reg, NULL);
> +		BUG_ON(r);
> +	}
> +
>  	r = dm_bufio_write_dirty_buffers(ic->bufio);
>  	if (unlikely(r))
>  		dm_integrity_io_error(ic, "writing tags", r);
> +
> +	if (flush_data)
> +		wait_for_completion(&fr.comp);
>  }
>  
>  static void sleep_on_endio_wait(struct dm_integrity_c *ic)
> @@ -2110,7 +2150,7 @@ offload_to_thread:
>  
>  	if (unlikely(dio->op == REQ_OP_DISCARD) && likely(ic->mode != 'D')) {
>  		integrity_metadata(&dio->work);
> -		dm_integrity_flush_buffers(ic);
> +		dm_integrity_flush_buffers(ic, false);
>  
>  		dio->in_flight = (atomic_t)ATOMIC_INIT(1);
>  		dio->completion = NULL;
> @@ -2195,7 +2235,7 @@ static void integrity_commit(struct work
>  	flushes = bio_list_get(&ic->flush_bio_list);
>  	if (unlikely(ic->mode != 'J')) {
>  		spin_unlock_irq(&ic->endio_wait.lock);
> -		dm_integrity_flush_buffers(ic);
> +		dm_integrity_flush_buffers(ic, true);
>  		goto release_flush_bios;
>  	}
>  
> @@ -2409,7 +2449,7 @@ skip_io:
>  	complete_journal_op(&comp);
>  	wait_for_completion_io(&comp.comp);
>  
> -	dm_integrity_flush_buffers(ic);
> +	dm_integrity_flush_buffers(ic, true);
>  }
>  
>  static void integrity_writer(struct work_struct *w)
> @@ -2451,7 +2491,7 @@ static void recalc_write_super(struct dm
>  {
>  	int r;
>  
> -	dm_integrity_flush_buffers(ic);
> +	dm_integrity_flush_buffers(ic, false);
>  	if (dm_integrity_failed(ic))
>  		return;
>  
> @@ -2654,7 +2694,7 @@ static void bitmap_flush_work(struct wor
>  	unsigned long limit;
>  	struct bio *bio;
>  
> -	dm_integrity_flush_buffers(ic);
> +	dm_integrity_flush_buffers(ic, false);
>  
>  	range.logical_sector = 0;
>  	range.n_sectors = ic->provided_data_sectors;
> @@ -2663,9 +2703,7 @@ static void bitmap_flush_work(struct wor
>  	add_new_range_and_wait(ic, &range);
>  	spin_unlock_irq(&ic->endio_wait.lock);
>  
> -	dm_integrity_flush_buffers(ic);
> -	if (ic->meta_dev)
> -		blkdev_issue_flush(ic->dev->bdev, GFP_NOIO);
> +	dm_integrity_flush_buffers(ic, true);
>  
>  	limit = ic->provided_data_sectors;
>  	if (ic->sb->flags & cpu_to_le32(SB_FLAG_RECALCULATING)) {
> @@ -2934,11 +2972,11 @@ static void dm_integrity_postsuspend(str
>  		if (ic->meta_dev)
>  			queue_work(ic->writer_wq, &ic->writer_work);
>  		drain_workqueue(ic->writer_wq);
> -		dm_integrity_flush_buffers(ic);
> +		dm_integrity_flush_buffers(ic, true);
>  	}
>  
>  	if (ic->mode == 'B') {
> -		dm_integrity_flush_buffers(ic);
> +		dm_integrity_flush_buffers(ic, true);
>  #if 1
>  		/* set to 0 to test bitmap replay code */
>  		init_journal(ic, 0, ic->journal_sections, 0);
> Index: linux-2.6/include/linux/dm-bufio.h
> ===================================================================
> --- linux-2.6.orig/include/linux/dm-bufio.h	2020-09-05 10:01:42.000000000 +0200
> +++ linux-2.6/include/linux/dm-bufio.h	2021-01-08 15:12:31.000000000 +0100
> @@ -150,6 +150,7 @@ void dm_bufio_set_minimum_buffers(struct
>  
>  unsigned dm_bufio_get_block_size(struct dm_bufio_client *c);
>  sector_t dm_bufio_get_device_size(struct dm_bufio_client *c);
> +struct dm_io_client *dm_bufio_get_dm_io_client(struct dm_bufio_client *c);
>  sector_t dm_bufio_get_block_number(struct dm_buffer *b);
>  void *dm_bufio_get_block_data(struct dm_buffer *b);
>  void *dm_bufio_get_aux_data(struct dm_buffer *b);
> Index: linux-2.6/drivers/md/dm-bufio.c
> ===================================================================
> --- linux-2.6.orig/drivers/md/dm-bufio.c	2021-01-08 15:11:20.000000000 +0100
> +++ linux-2.6/drivers/md/dm-bufio.c	2021-01-08 15:12:25.000000000 +0100
> @@ -1534,6 +1534,12 @@ sector_t dm_bufio_get_device_size(struct
>  }
>  EXPORT_SYMBOL_GPL(dm_bufio_get_device_size);
>  
> +struct dm_io_client *dm_bufio_get_dm_io_client(struct dm_bufio_client *c)
> +{
> +	return c->dm_io;
> +}
> +EXPORT_SYMBOL_GPL(dm_bufio_get_dm_io_client);
> +
>  sector_t dm_bufio_get_block_number(struct dm_buffer *b)
>  {
>  	return b->block;
> 



-- 


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 93 bytes --]

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] dm-integrity: Fix flush with external metadata device

[Mike Snitzer]

On Fri, Jan 08 2021 at 11:12am -0500,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> 
> 
> On Mon, 4 Jan 2021, Mike Snitzer wrote:
> 
> > On Sun, Dec 20 2020 at  8:02am -0500,
> > Lukas Straub <lukasstraub2@web.de> wrote:
> > 
> > > With an external metadata device, flush requests aren't passed down
> > > to the data device.
> > > 
> > > Fix this by issuing flush in the right places: In integrity_commit
> > > when not in journal mode, in do_journal_write after writing the
> > > contents of the journal to the disk and in dm_integrity_postsuspend.
> > > 
> > > Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> > > ---
> > >  drivers/md/dm-integrity.c | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > > 
> > > diff --git a/drivers/md/dm-integrity.c b/drivers/md/dm-integrity.c
> > > index 5a7a1b90e671..a26ed65869f6 100644
> > > --- a/drivers/md/dm-integrity.c
> > > +++ b/drivers/md/dm-integrity.c
> > > @@ -2196,6 +2196,8 @@ static void integrity_commit(struct work_struct *w)
> > >  	if (unlikely(ic->mode != 'J')) {
> > >  		spin_unlock_irq(&ic->endio_wait.lock);
> > >  		dm_integrity_flush_buffers(ic);
> > > +		if (ic->meta_dev)
> > > +			blkdev_issue_flush(ic->dev->bdev, GFP_NOIO);
> > >  		goto release_flush_bios;
> > >  	}
> > >  
> > > @@ -2410,6 +2412,9 @@ static void do_journal_write(struct dm_integrity_c *ic, unsigned write_start,
> > >  	wait_for_completion_io(&comp.comp);
> > >  
> > >  	dm_integrity_flush_buffers(ic);
> > > +	if (ic->meta_dev)
> > > +		blkdev_issue_flush(ic->dev->bdev, GFP_NOIO);
> > > +
> > >  }
> > >  
> > >  static void integrity_writer(struct work_struct *w)
> > > @@ -2949,6 +2954,9 @@ static void dm_integrity_postsuspend(struct dm_target *ti)
> > >  #endif
> > >  	}
> > >  
> > > +	if (ic->meta_dev)
> > > +		blkdev_issue_flush(ic->dev->bdev, GFP_NOIO);
> > > +
> > >  	BUG_ON(!RB_EMPTY_ROOT(&ic->in_progress));
> > >  
> > >  	ic->journal_uptodate = true;
> > > -- 
> > > 2.20.1
> > 
> > 
> > Seems like a pretty bad oversight... but shouldn't you also make sure to
> > flush the data device _before_ the metadata is flushed?
> > 
> > Mike
> 
> I think, ordering is not a problem.
> 
> A disk may flush its cache spontaneously anytime, so it doesn't matter in 
> which order do we flush them. Similarly a dm-bufio buffer may be flushed 
> anytime - if the machine is running out of memory and a dm-bufio shrinker 
> is called.
> 
> I'll send another patch for this - I've created a patch that flushes the 
> metadata device cache and data device cache in parallel, so that 
> performance degradation is reduced.
> 
> My patch also doesn't use GFP_NOIO allocation - which can in theory 
> deadlock if we are swapping on dm-integrity device.

OK, I see your patch, but my concern about ordering was more to do with
crash consistency.  What if metadata is updated but data isn't?

On the surface, your approach of issuing the flushes in parallel seems
to expose us to inconsistency upon recovery from a crash.
If that isn't the case please explain why not.

Thanks,
Mike

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] dm-integrity: Fix flush with external metadata device

[Mikulas Patocka]

On Fri, 8 Jan 2021, Mike Snitzer wrote:

> > > Seems like a pretty bad oversight... but shouldn't you also make sure to
> > > flush the data device _before_ the metadata is flushed?
> > > 
> > > Mike
> > 
> > I think, ordering is not a problem.
> > 
> > A disk may flush its cache spontaneously anytime, so it doesn't matter in 
> > which order do we flush them. Similarly a dm-bufio buffer may be flushed 
> > anytime - if the machine is running out of memory and a dm-bufio shrinker 
> > is called.
> > 
> > I'll send another patch for this - I've created a patch that flushes the 
> > metadata device cache and data device cache in parallel, so that 
> > performance degradation is reduced.
> > 
> > My patch also doesn't use GFP_NOIO allocation - which can in theory 
> > deadlock if we are swapping on dm-integrity device.
> 
> OK, I see your patch, but my concern about ordering was more to do with
> crash consistency.  What if metadata is updated but data isn't?

In journal mode: do_journal_write will copy data from the journal to 
appropriate places and then flushes both data and metadata device. If a 
crash happens during flush, the journal will be replayed on next reboot - 
the replay operation will overwrite both data and metadata from the 
journal.

In bitmap mode: bitmap_flush_work will first issue flush on both data and 
metadata device, and then clear the dirty bits in a bitmap. If a crash 
happens during the flush, the bits in the bitmap are still set and the 
checksums will be recalculated on next reboot.

> On the surface, your approach of issuing the flushes in parallel seems
> to expose us to inconsistency upon recovery from a crash.
> If that isn't the case please explain why not.

The disk may flush its internal cache anytime. So, if you do 
"blkdev_issue_flush(disk1); blkdev_issue_flush(disk2);" there is no 
ordering guarantee at all. The firmware in disk2 may flush the cache 
spontaneously, before you called blkdev_issue_flush(disk1).

> Thanks,
> Mike

Mikulas

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel