Re: [dm-devel] [PATCH] dm-bufio

[Mikulas Patocka <mpatocka redhat com>]

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