[dm-devel] [PATCH] dm-bufio

[Mikulas Patocka]

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