[PATCH] dm-bufio: implement discard

[PATCH] dm-bufio: implement discard

[Mikulas Patocka]

Add functions dm_bufio_issue_discard and dm_bufio_discard_buffers.
dm_bufio_issue_discard sends discard request to the underlying device.
dm_bufio_discard_buffers frees buffers in the range and then calls
dm_bufio_issue_discard.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 drivers/md/dm-bufio.c    |   50 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/dm-bufio.h |   12 +++++++++++
 2 files changed, 62 insertions(+)

Index: linux-2.6/drivers/md/dm-bufio.c
===================================================================
--- linux-2.6.orig/drivers/md/dm-bufio.c	2020-02-07 17:14:08.000000000 +0100
+++ linux-2.6/drivers/md/dm-bufio.c	2020-02-07 17:29:42.000000000 +0100
@@ -1338,6 +1338,56 @@ int dm_bufio_issue_flush(struct dm_bufio
 EXPORT_SYMBOL_GPL(dm_bufio_issue_flush);
 
 /*
+ * Use dm-io to send a discard request to flush the device.
+ */
+int dm_bufio_issue_discard(struct dm_bufio_client *c, sector_t block, sector_t count)
+{
+	struct dm_io_request io_req = {
+		.bi_op = REQ_OP_DISCARD,
+		.bi_op_flags = REQ_SYNC,
+		.mem.type = DM_IO_KMEM,
+		.mem.ptr.addr = NULL,
+		.client = c->dm_io,
+	};
+	struct dm_io_region io_reg = {
+		.bdev = c->bdev,
+		.sector = block_to_sector(c, block),
+		.count = block_to_sector(c, count),
+	};
+
+	BUG_ON(dm_bufio_in_request());
+
+	return dm_io(&io_req, 1, &io_reg, NULL);
+}
+EXPORT_SYMBOL_GPL(dm_bufio_issue_discard);
+
+/*
+ * Free the specified range of buffers. If a buffer is held by other process, it
+ * is not freed. If a buffer is dirty, it is discarded without writeback.
+ * Finally, send the discard request to the device.
+ */
+int dm_bufio_discard_buffers(struct dm_bufio_client *c, sector_t block, sector_t count)
+{
+	sector_t i;
+
+	for (i = block; i < block + count; i++) {
+		struct dm_buffer *b;
+		dm_bufio_lock(c);
+		b = __find(c, i);
+		if (b && likely(!b->hold_count)) {
+			wait_on_bit_io(&b->state, B_READING, TASK_UNINTERRUPTIBLE);
+			wait_on_bit_io(&b->state, B_WRITING, TASK_UNINTERRUPTIBLE);
+			__unlink_buffer(b);
+			__free_buffer_wake(b);
+		}
+		dm_bufio_unlock(c);
+	}
+
+	return dm_bufio_issue_discard(c, block, count);
+}
+EXPORT_SYMBOL_GPL(dm_bufio_discard_buffers);
+
+/*
  * We first delete any other buffer that may be at that new location.
  *
  * Then, we write the buffer to the original location if it was dirty.
Index: linux-2.6/include/linux/dm-bufio.h
===================================================================
--- linux-2.6.orig/include/linux/dm-bufio.h	2020-02-07 17:14:08.000000000 +0100
+++ linux-2.6/include/linux/dm-bufio.h	2020-02-07 17:29:46.000000000 +0100
@@ -126,6 +126,18 @@ int dm_bufio_write_dirty_buffers(struct
 int dm_bufio_issue_flush(struct dm_bufio_client *c);
 
 /*
+ * Send a discard request to the underlying device.
+ */
+int dm_bufio_issue_discard(struct dm_bufio_client *c, sector_t block, sector_t count);
+
+/*
+ * Free the specified range of buffers. If a buffer is held by other process, it
+ * is not freed. If a buffer is dirty, it is discarded without writeback.
+ * Finally, send the discard request to the device.
+ */
+int dm_bufio_discard_buffers(struct dm_bufio_client *c, sector_t block, sector_t count);
+
+/*
  * Like dm_bufio_release but also move the buffer to the new
  * block. dm_bufio_write_dirty_buffers is needed to commit the new block.
  */

Re: [PATCH] dm-bufio: implement discard

[John Dorminy]

> +/*
> + * Free the specified range of buffers. If a buffer is held by other process, it
> + * is not freed. If a buffer is dirty, it is discarded without writeback.
> + * Finally, send the discard request to the device.
Might be clearer to say "After freeing, send a discard request for the
specified range to the device." to clarify that it's all cases, not
just the dirty-buffer case mentioned in the previous sentence.

> + */
> +int dm_bufio_discard_buffers(struct dm_bufio_client *c, sector_t block, sector_t count)
> +{
> +       sector_t i;
> +
> +       for (i = block; i < block + count; i++) {
> +               struct dm_buffer *b;
> +               dm_bufio_lock(c);
> +               b = __find(c, i);
> +               if (b && likely(!b->hold_count)) {
> +                       wait_on_bit_io(&b->state, B_READING, TASK_UNINTERRUPTIBLE);
> +                       wait_on_bit_io(&b->state, B_WRITING, TASK_UNINTERRUPTIBLE);
> +                       __unlink_buffer(b);
> +                       __free_buffer_wake(b);
> +               }
> +               dm_bufio_unlock(c);
> +       }
> +
> +       return dm_bufio_issue_discard(c, block, count);
> +}
> +EXPORT_SYMBOL_GPL(dm_bufio_discard_buffers);

This seems dangerous -- if another process is holding the buffer, you
could be issuing a discard while they are reading or writing, or vice
versa.

Discards whose lifetime overlaps with the lifetime of a read or write
to the same region have undefined behavior, as far as I know.

Perhaps dm_bufio_issue_discard() should allow the caller to specify a
callback, in which case dm_bufio_discard_buffers could unlock any
buffers in the range upon completion; or dm_bufio_discard_buffers
should not issue discards for blocks held by other processes.

Re: [PATCH] dm-bufio: implement discard

[Mikulas Patocka]

On Fri, 7 Feb 2020, John Dorminy wrote:

> > +/*
> > + * Free the specified range of buffers. If a buffer is held by other process, it
> > + * is not freed. If a buffer is dirty, it is discarded without writeback.
> > + * Finally, send the discard request to the device.
> Might be clearer to say "After freeing, send a discard request for the
> specified range to the device." to clarify that it's all cases, not
> just the dirty-buffer case mentioned in the previous sentence.
> 
> > + */
> > +int dm_bufio_discard_buffers(struct dm_bufio_client *c, sector_t block, sector_t count)
> > +{
> > +       sector_t i;
> > +
> > +       for (i = block; i < block + count; i++) {
> > +               struct dm_buffer *b;
> > +               dm_bufio_lock(c);
> > +               b = __find(c, i);
> > +               if (b && likely(!b->hold_count)) {
> > +                       wait_on_bit_io(&b->state, B_READING, TASK_UNINTERRUPTIBLE);
> > +                       wait_on_bit_io(&b->state, B_WRITING, TASK_UNINTERRUPTIBLE);
> > +                       __unlink_buffer(b);
> > +                       __free_buffer_wake(b);
> > +               }
> > +               dm_bufio_unlock(c);
> > +       }
> > +
> > +       return dm_bufio_issue_discard(c, block, count);
> > +}
> > +EXPORT_SYMBOL_GPL(dm_bufio_discard_buffers);
> 
> This seems dangerous -- if another process is holding the buffer, you
> could be issuing a discard while they are reading or writing, or vice
> versa.
> 
> Discards whose lifetime overlaps with the lifetime of a read or write
> to the same region have undefined behavior, as far as I know.

So the user should not do it. I don't see a problem with it. If the user 
sends two ovelapping writes, it is unspecified which of them stays on the 
device. If the user sends a read with overlapping write, it is unspecified 
if the read returns the old data or the new data.

It's the same with overlapping discards and other I/Os.

> Perhaps dm_bufio_issue_discard() should allow the caller to specify a
> callback, in which case dm_bufio_discard_buffers could unlock any
> buffers in the range upon completion; or dm_bufio_discard_buffers
> should not issue discards for blocks held by other processes.

Mikulas

Re: [PATCH] dm-bufio: implement discard

[John Dorminy]

On Fri, Feb 7, 2020 at 1:04 PM Mikulas Patocka <mpatocka@redhat.com> wrote:
>
>
>
> On Fri, 7 Feb 2020, John Dorminy wrote:
>
> > > +/*
> > > + * Free the specified range of buffers. If a buffer is held by other process, it
> > > + * is not freed. If a buffer is dirty, it is discarded without writeback.
> > > + * Finally, send the discard request to the device.
> > Might be clearer to say "After freeing, send a discard request for the
> > specified range to the device." to clarify that it's all cases, not
> > just the dirty-buffer case mentioned in the previous sentence.
> >
> > > + */
> > > +int dm_bufio_discard_buffers(struct dm_bufio_client *c, sector_t block, sector_t count)
> > > +{
> > > +       sector_t i;
> > > +
> > > +       for (i = block; i < block + count; i++) {
> > > +               struct dm_buffer *b;
> > > +               dm_bufio_lock(c);
> > > +               b = __find(c, i);
> > > +               if (b && likely(!b->hold_count)) {
> > > +                       wait_on_bit_io(&b->state, B_READING, TASK_UNINTERRUPTIBLE);
> > > +                       wait_on_bit_io(&b->state, B_WRITING, TASK_UNINTERRUPTIBLE);
> > > +                       __unlink_buffer(b);
> > > +                       __free_buffer_wake(b);
> > > +               }
> > > +               dm_bufio_unlock(c);
> > > +       }
> > > +
> > > +       return dm_bufio_issue_discard(c, block, count);
> > > +}
> > > +EXPORT_SYMBOL_GPL(dm_bufio_discard_buffers);
> >
> > This seems dangerous -- if another process is holding the buffer, you
> > could be issuing a discard while they are reading or writing, or vice
> > versa.
> >
> > Discards whose lifetime overlaps with the lifetime of a read or write
> > to the same region have undefined behavior, as far as I know.
>
> So the user should not do it. I don't see a problem with it. If the user
> sends two ovelapping writes, it is unspecified which of them stays on the
> device. If the user sends a read with overlapping write, it is unspecified
> if the read returns the old data or the new data.

There's no obvious way for the other process to wait for the discard
to be complete at the present time, though -- suppose there were two
holders of a given buffer, and one decided to discard -- how will the
second one to wait for the discard to complete, or even tell that it's
currently being discarded from another thread? I would naively guess
that __write_dirty_buffer() waits via wait_on_bit_io(&b->state,
B_WRITING, ...) to make sure nobody else is doing IO on the buffer
location at the moment, but a discard doesn't currently set that bit,
as far as I can see.

(If there is a way to wait, perhaps it should be documented at
dm_bufio_discard_buffers() -- "If there is another process holding the
buffer, the other process should be sure to [do stuff] before issuing
a write, lest the write potentially be dropped or corrupted."

Re: [PATCH] dm-bufio: implement discard

[Mikulas Patocka]

On Fri, 7 Feb 2020, John Dorminy wrote:

> There's no obvious way for the other process to wait for the discard
> to be complete at the present time, though -- suppose there were two
> holders of a given buffer, and one decided to discard -- how will the
> second one to wait for the discard to complete, or even tell that it's
> currently being discarded from another thread? I would naively guess
> that __write_dirty_buffer() waits via wait_on_bit_io(&b->state,
> B_WRITING, ...) to make sure nobody else is doing IO on the buffer
> location at the moment, but a discard doesn't currently set that bit,
> as far as I can see.
> 
> (If there is a way to wait, perhaps it should be documented at
> dm_bufio_discard_buffers() -- "If there is another process holding the
> buffer, the other process should be sure to [do stuff] before issuing
> a write, lest the write potentially be dropped or corrupted."

If two processes write to the same buffer, it is undefined behavior. If 
both of them do this:
	1) dm_bufio_get
	2) ... write to the buffer
	3) dm_bufio_mark_buffer_dirty
	4) dm_bufio_release
it is undefined what data the buffer would hold. It can even hold mixture 
of data written by those two processes. You must design your code in such 
a way that this doesn't happen.

The same is with discards - if you want to use them, you must design your 
code so that it doesn't overlay discards with other I/O. If you can't 
design it this way, then don't use discard.

Mikulas

Re: [PATCH] dm-bufio: implement discard

[John Dorminy]

Yeah, that's a great point. Now that I've reviewed the code a little
more, I understand how it's not safe to do the thing I described in
the first place, and how this change is safe and correct.

Please feel free to add my

Reviewed-by: John Dorminy <jdorminy@redhat.com>

Thanks!


On Fri, Feb 7, 2020 at 3:07 PM Mikulas Patocka <mpatocka@redhat.com> wrote:
>
>
>
> On Fri, 7 Feb 2020, John Dorminy wrote:
>
> > There's no obvious way for the other process to wait for the discard
> > to be complete at the present time, though -- suppose there were two
> > holders of a given buffer, and one decided to discard -- how will the
> > second one to wait for the discard to complete, or even tell that it's
> > currently being discarded from another thread? I would naively guess
> > that __write_dirty_buffer() waits via wait_on_bit_io(&b->state,
> > B_WRITING, ...) to make sure nobody else is doing IO on the buffer
> > location at the moment, but a discard doesn't currently set that bit,
> > as far as I can see.
> >
> > (If there is a way to wait, perhaps it should be documented at
> > dm_bufio_discard_buffers() -- "If there is another process holding the
> > buffer, the other process should be sure to [do stuff] before issuing
> > a write, lest the write potentially be dropped or corrupted."
>
> If two processes write to the same buffer, it is undefined behavior. If
> both of them do this:
>         1) dm_bufio_get
>         2) ... write to the buffer
>         3) dm_bufio_mark_buffer_dirty
>         4) dm_bufio_release
> it is undefined what data the buffer would hold. It can even hold mixture
> of data written by those two processes. You must design your code in such
> a way that this doesn't happen.
>
> The same is with discards - if you want to use them, you must design your
> code so that it doesn't overlay discards with other I/O. If you can't
> design it this way, then don't use discard.
>
> Mikulas
>