[dm-devel] [PATCH v2 7/7] dm snapshot: use bufio prefetch

[Mikulas Patocka]

On Mon, 13 Jan 2014, Mike Snitzer wrote:

> On Mon, Jan 13 2014 at  6:45pm -0500,
> Mikulas Patocka <mpatocka at redhat.com> wrote:
> 
> > 
> > 
> > On Mon, 13 Jan 2014, Mike Snitzer wrote:
> > 
> > > On Mon, Jan 13 2014 at  5:00pm -0500,
> > > Mikulas Patocka <mpatocka at redhat.com> wrote:
> > > 
> > > > No.
> > > > 
> > > > This changed patch inefficiently loops in bufio_prefetch_chunks for each 
> > > > buffer that is read.
> > > 
> > > Yeah, I overlooked the importance of preserving prefetch_area (which
> > > explains the unlikely you had).  But my intent was to make your code
> > > less "special"; your if (DM_PREFETCH_CHUNKS) do { } while() code to
> > > avoid the extra indentation is pretty ugly.
> > > 
> > > Given your current code, DM_PREFETCH_CHUNKS is always 12 so why not just
> > > remove the check?
> > 
> > If someone ever changes it to a variable, the condition is there avoid 
> > unneeded call to dm_bufio_prefetch. The compiler optimizes out the 
> > constant expression, so it doesn't matter that it's there.
> 
> How about move the conditional inside the do { } ?
> 
> I really dislike the style you've used on this, is there anywhere else
> in the kernel that does this?

grep -r 'if (.*) do {' *

shows

arch/hexagon/include/asm/cmpxchg.h:     if (size != 4) do { asm volatile("brkpt;\n"); } while (1);
drivers/media/usb/tm6000/tm6000-i2c.c:#define i2c_dprintk(lvl, fmt, args...) if (i2c_debug >= lvl) do { \
drivers/net/wireless/airo.c:    if (rc == SUCCESS) do {
drivers/net/wireless/airo.c:    if (rc == SUCCESS) do {
include/linux/zutil.h:        if (k != 0) do {
lib/lzo/lzo1x_compress.c:                               if (t > 0) do {
lib/lzo/lzo1x_compress.c:               if (t >= 16) do {
lib/lzo/lzo1x_compress.c:               if (t > 0) do {
lib/zlib_deflate/deftree.c:    if (s->last_lit != 0) do {

I don't think it's that problematic.

Mikulas