Re: [PATCH] dm-bufio: implement discard

[Mikulas Patocka <mpatocka@redhat.com>]

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