On Fri, Oct 14, 2011 at 03:14:34PM -0400, Mikulas Patocka wrote:
@@ -999,15 +1013,19 @@ again:
* someone is doing some writes simultaneously with us --- in
* this case, stop dropping the lock.
*/
if (dropped_lock)
goto again;
}
wake_up(&c->free_buffer_wait);
dm_bufio_unlock(c);

- a = xchg(&c->async_write_error, 0);
+ if (likely(!c->async_write_error))
+ a = 0;
+ else
+ a = xchg(&c->async_write_error, 0);
f = dm_bufio_issue_flush(c);


I didn't change this. Is it also fixing a preexisting issue?

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

On Fri, Oct 14, 2011 at 03:14:34PM -0400, Mikulas Patocka wrote:
> Hi
>
> This is a patch for dm-bufio.

I've merged all I'm going to at this point and pushed to thin-dev.

If you put together a patch for the cond_resched stuff I'll take that;
providing you don't call it in a tight loop like here:

> @@ -758,8 +772,9 @@ static struct dm_buffer *__find(struct d
> struct dm_buffer *b;
> struct hlist_node *hn;
> hlist_for_each_entry(b, hn, &c->cache_hash[DM_BUFIO_HASH(block)], hash_list) {
> - if (b->block == block)
> + if (likely(b->block == block))
> return b;
> + dm_bufio_cond_resched();
> }


I'm not really interested in the likely/unlikely annotations.

- Joe

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

On Mon, Oct 17 2011 at 6:08am -0400,
Joe Thornber <thornber@redhat.com> wrote:

> On Fri, Oct 14, 2011 at 03:14:34PM -0400, Mikulas Patocka wrote:
> > Hi
> >
> > This is a patch for dm-bufio.
> >
> > Changes:
...
> > * dm-bufio.h moved from drivers/md to include/linux
>
> Who outside dm do you expect to use this?

External DM target modules may have an interest (not great justification
but...).

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

On Mon, 17 Oct 2011, Joe Thornber wrote:

> On Fri, Oct 14, 2011 at 03:14:34PM -0400, Mikulas Patocka wrote:
> @@ -999,15 +1013,19 @@ again:
> * someone is doing some writes simultaneously with us --- in
> * this case, stop dropping the lock.
> */
> if (dropped_lock)
> goto again;
> }
> wake_up(&c->free_buffer_wait);
> dm_bufio_unlock(c);
>
> - a = xchg(&c->async_write_error, 0);
> + if (likely(!c->async_write_error))
> + a = 0;
> + else
> + a = xchg(&c->async_write_error, 0);
> f = dm_bufio_issue_flush(c);
>
>
> I didn't change this. Is it also fixing a preexisting issue?

It doesn't fix anything. It just bypasses xchg for the most common case.

Mikulas

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

On Mon, 17 Oct 2011, Joe Thornber wrote:

> On Fri, Oct 14, 2011 at 03:14:34PM -0400, Mikulas Patocka wrote:
> > -static struct dm_buffer *__bufio_new(struct dm_bufio_client *c, sector_t block, enum new_flag nf,
> > - struct dm_buffer **bp, int *need_submit)
> > +static void read_endio(struct bio *bio, int error);
> > +
> > +static struct dm_buffer *__bufio_new(struct dm_bufio_client *c, sector_t block,
> > + enum new_flag nf, struct dm_buffer **bp)
> > {
> > struct dm_buffer *b, *new_b = NULL;
> >
> > - *need_submit = 0;
> > b = __find(c, block);
> > if (b) {
> > b->hold_count++;
> > @@ -821,7 +837,9 @@ static struct dm_buffer *__bufio_new(str
> > }
> >
> > b->state = 1 << B_READING;
> > - *need_submit = 1;
> > +
> > + submit_io(b, READ, b->block, read_endio);
> > +
> > return b;
> > }
> >
> > @@ -849,22 +867,18 @@ static void read_endio(struct bio *bio,
> > static void *new_read(struct dm_bufio_client *c, sector_t block, enum new_flag nf,
> > struct dm_buffer **bp)
> > {
> > - int need_submit;
> > struct dm_buffer *b;
> >
> > dm_bufio_lock(c);
> > - b = __bufio_new(c, block, nf, bp, &need_submit);
> > + b = __bufio_new(c, block, nf, bp);
> > dm_bufio_unlock(c);
> >
> > if (!b || IS_ERR(b))
> > return b;
> > else {
> > - if (need_submit)
> > - submit_io(b, READ, b->block, read_endio);
> > -
> > wait_on_bit(&b->state, B_READING,
> > do_io_schedule, TASK_UNINTERRUPTIBLE);
>
>
> This change means submit_io(), which can block, will be called with
> the client lock held. I don't see this as an improvement. NACK.

dm-bufio is designed with the possibility that submit_io is called inside
the lock. There are other places that do it too. But it's true that it
wasn't called in the lock before.

I think the best thing would be to delete those functions and use the code
that was there before.

Mikulas

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

On Mon, Oct 17, 2011 at 09:47:05AM -0400, Mikulas Patocka wrote:
> I think the best thing would be to delete those functions and use the code
> that was there before.

I know you do. However, Alasdair made it clear that he was not
willing to push the code as it was and we don't have time to argue.

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

On Mon, 17 Oct 2011, Joe Thornber wrote:

> On Fri, Oct 14, 2011 at 03:14:34PM -0400, Mikulas Patocka wrote:
> > Hi
> >
> > This is a patch for dm-bufio.
> >
> > Changes:
> > * fixed a bug in i/o submission, introduced by someone who changed the
> > code
>
> Could you point me at the specific part of this patch that does this
> please?

this:
BUG_ON(b->c->block_size <= PAGE_SIZE);
use_dmio(b, rw, block, end_io);
+ return;


> > * simplified some constructs
> > * more likely/unlikely hints
>
> They clutter the code, and have been used without discrimination. What
> is the point of using it on a slow path?

I think I added them only to the possible hot paths.

> > * dm-bufio.h moved from drivers/md to include/linux
>
> Who outside dm do you expect to use this?

I don't know, Alasdair said that he wants it there (like
include/linux/dm-io.h for example). BTW dm-bufio has nothing to do with
device mapper actually, so it can be used by any code.

> > * put cond_resched() to loops (it was there originally and then someone
> > deleted it)
>
> If you're going to use cond_resched() at least do so a little more
> intelligently than putting it in _every_ loop. For instance you call it on
> every iteration of a sweep through the hash table. The call to
> cond_resched will take more time than the loop body. At least make a
> change so it's only done every n'th iteration.

I think it would be better to use
#ifndef CONFIG_PREEMPT
if (need_resched()) cond_resched();
#endif

need_resched() is inlined and translates to a single condition.

I don't know why Linux doesn't provide a macro for it, this would be
useful far beyond dm code.

Mikulas

> Can I also point out that I asked you to make a lot of these changes
> over a year ago. You've only got yourself to blame if 'someone' does
> it for you.
>
> - Someone

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

On Mon, 17 Oct 2011, Joe Thornber wrote:

> On Fri, Oct 14, 2011 at 03:14:34PM -0400, Mikulas Patocka wrote:
> @@ -493,8 +500,10 @@ static void use_inline_bio(struct dm_buf
> static void submit_io(struct dm_buffer *b, int rw, sector_t block,
> bio_end_io_t *end_io)
> {
> - if (b->c->block_size <= DM_BUFIO_INLINE_VECS * PAGE_SIZE &&
> - b->data_mode != DATA_MODE_VMALLOC)
> + if (rw == WRITE && b->c->write_callback)
> + b->c->write_callback(b);
> if (likely(b->c->block_size <= DM_BUFIO_INLINE_VECS * PAGE_SIZE) &&
> likely(b->data_mode != DATA_MODE_VMALLOC))
> use_inline_bio(b, rw, block, end_io);
> else
> use_dmio(b, rw, block, end_io);
> @@ -550,8 +559,6 @@ static void __write_dirty_buffer(struct
> clear_bit(B_DIRTY, &b->state);
> wait_on_bit_lock(&b->state, B_WRITING,
> do_io_schedule, TASK_UNINTERRUPTIBLE);
> - if (b->c->write_callback)
> - b->c->write_callback(b);
> submit_io(b, WRITE, b->block, write_endio);
> }
>
>
> This doesn't seem an improvement. Except ... it changes the behaviour
> of dm_bufio_release_move(). So was there a preexisting bug in
> dm_bufio_release_move() that you're trying to fix with this patch?

The actual reason was to do this callback in dm_bufio_release_move() too
--- just for consistency. (the user of dm_bufio_release_move() doesn't use
write_callback anyway).

Mikulas

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

On Mon, Oct 17, 2011 at 10:05:26AM -0400, Mikulas Patocka wrote:
>
>
> On Mon, 17 Oct 2011, Joe Thornber wrote:
>
> > On Fri, Oct 14, 2011 at 03:14:34PM -0400, Mikulas Patocka wrote:
> > @@ -493,8 +500,10 @@ static void use_inline_bio(struct dm_buf
> > static void submit_io(struct dm_buffer *b, int rw, sector_t block,
> > bio_end_io_t *end_io)
> > {
> > - if (b->c->block_size <= DM_BUFIO_INLINE_VECS * PAGE_SIZE &&
> > - b->data_mode != DATA_MODE_VMALLOC)
> > + if (rw == WRITE && b->c->write_callback)
> > + b->c->write_callback(b);
> > if (likely(b->c->block_size <= DM_BUFIO_INLINE_VECS * PAGE_SIZE) &&
> > likely(b->data_mode != DATA_MODE_VMALLOC))
> > use_inline_bio(b, rw, block, end_io);
> > else
> > use_dmio(b, rw, block, end_io);
> > @@ -550,8 +559,6 @@ static void __write_dirty_buffer(struct
> > clear_bit(B_DIRTY, &b->state);
> > wait_on_bit_lock(&b->state, B_WRITING,
> > do_io_schedule, TASK_UNINTERRUPTIBLE);
> > - if (b->c->write_callback)
> > - b->c->write_callback(b);
> > submit_io(b, WRITE, b->block, write_endio);
> > }
> >
> >
> > This doesn't seem an improvement. Except ... it changes the behaviour
> > of dm_bufio_release_move(). So was there a preexisting bug in
> > dm_bufio_release_move() that you're trying to fix with this patch?
>
> The actual reason was to do this callback in dm_bufio_release_move() too
> --- just for consistency. (the user of dm_bufio_release_move() doesn't use
> write_callback anyway).

thinp uses dm_bufio_release_move() and write_callback. So yes, this
is a bug fix. I thought so and merged.

- Joe

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

On Mon, Oct 17, 2011 at 10:04:16AM -0400, Mikulas Patocka wrote:
> > If you're going to use cond_resched() at least do so a little more
> > intelligently than putting it in _every_ loop. For instance you call it on
> > every iteration of a sweep through the hash table. The call to
> > cond_resched will take more time than the loop body. At least make a
> > change so it's only done every n'th iteration.
>
> I think it would be better to use
> #ifndef CONFIG_PREEMPT
> if (need_resched()) cond_resched();
> #endif
>
> need_resched() is inlined and translates to a single condition.

Yep, that would be fine.

> I don't know why Linux doesn't provide a macro for it, this would be
> useful far beyond dm code.

Agreed, I was very surprised that cond_resched() expands out to a
function call rather than a test + fn call.

Get a patch together and I'll merge.

- Joe

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