| Message ID | 20140113213758.GC3268@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
No. This changed patch inefficiently loops in bufio_prefetch_chunks for each buffer that is read. Mikulas On Mon, 13 Jan 2014, Mike Snitzer wrote: > From: Mikulas Patocka <mpatocka@redhat.com> > > This patch modifies dm-snapshot so that it prefetches the buffers when > loading the exceptions. > > The number of chunk-sized buffers read ahead is specified in the > DM_PREFETCH_CHUNKS macro. The current default for DM_PREFETCH_CHUNKS > (12) may need to be adjusted to improve performance on different types > of storage -- a future patch should make this configurable. > > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com> > Signed-off-by: Mike Snitzer <snitzer@redhat.com> > --- > drivers/md/dm-snap-persistent.c | 34 +++++++++++++++++++++++++++++++++- > 1 files changed, 33 insertions(+), 1 deletions(-) > > diff --git a/drivers/md/dm-snap-persistent.c b/drivers/md/dm-snap-persistent.c > index 1692750..ea45b3b 100644 > --- a/drivers/md/dm-snap-persistent.c > +++ b/drivers/md/dm-snap-persistent.c > @@ -18,6 +18,8 @@ > #define DM_MSG_PREFIX "persistent snapshot" > #define DM_CHUNK_SIZE_DEFAULT_SECTORS 32 /* 16KB */ > > +#define DM_PREFETCH_CHUNKS 12 > + > /*----------------------------------------------------------------- > * Persistent snapshots, by persistent we mean that the snapshot > * will survive a reboot. > @@ -490,6 +492,30 @@ static int insert_exceptions(struct pstore *ps, void *ps_area, > return 0; > } > > +static void bufio_prefetch_chunks(struct dm_bufio_client *client, > + struct pstore *ps) > +{ > + chunk_t prefetch_area = 0; > + chunk_t pf_chunk; > + > + if (!DM_PREFETCH_CHUNKS) > + return; > + > + if (prefetch_area < ps->current_area) > + prefetch_area = ps->current_area; > + > + do { > + pf_chunk = area_location(ps, prefetch_area); > + if (unlikely(pf_chunk >= dm_bufio_get_device_size(client))) > + break; > + if (unlikely(!dm_bufio_prefetch(client, pf_chunk, 1))) > + break; > + prefetch_area++; > + if (unlikely(!prefetch_area)) > + break; > + } while (prefetch_area <= ps->current_area + DM_PREFETCH_CHUNKS); > +} > + > static int read_exceptions(struct pstore *ps, > int (*callback)(void *callback_context, chunk_t old, > chunk_t new), > @@ -505,6 +531,8 @@ static int read_exceptions(struct pstore *ps, > if (IS_ERR(client)) > return PTR_ERR(client); > > + dm_bufio_set_minimum_buffers(client, DM_PREFETCH_CHUNKS + 1); > + > /* > * Keeping reading chunks and inserting exceptions until > * we find a partially full area. > @@ -512,7 +540,11 @@ static int read_exceptions(struct pstore *ps, > for (ps->current_area = 0; full; ps->current_area++) { > struct dm_buffer *bp; > void *area; > - chunk_t chunk = area_location(ps, ps->current_area); > + chunk_t chunk; > + > + bufio_prefetch_chunks(client, ps); > + > + chunk = area_location(ps, ps->current_area); > > area = dm_bufio_read(client, chunk, &bp); > if (unlikely(IS_ERR(area))) { > -- > 1.7.4.4 > -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Mon, Jan 13 2014 at 5:00pm -0500, Mikulas Patocka <mpatocka@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? -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Mon, 13 Jan 2014, Mike Snitzer wrote: > On Mon, Jan 13 2014 at 5:00pm -0500, > Mikulas Patocka <mpatocka@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. Mikulas -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Mon, Jan 13 2014 at 6:45pm -0500, Mikulas Patocka <mpatocka@redhat.com> wrote: > > > On Mon, 13 Jan 2014, Mike Snitzer wrote: > > > On Mon, Jan 13 2014 at 5:00pm -0500, > > Mikulas Patocka <mpatocka@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? -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
On Mon, 13 Jan 2014, Mike Snitzer wrote: > On Mon, Jan 13 2014 at 6:45pm -0500, > Mikulas Patocka <mpatocka@redhat.com> wrote: > > > > > > > On Mon, 13 Jan 2014, Mike Snitzer wrote: > > > > > On Mon, Jan 13 2014 at 5:00pm -0500, > > > Mikulas Patocka <mpatocka@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 -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
diff --git a/drivers/md/dm-snap-persistent.c b/drivers/md/dm-snap-persistent.c index 1692750..ea45b3b 100644 --- a/drivers/md/dm-snap-persistent.c +++ b/drivers/md/dm-snap-persistent.c @@ -18,6 +18,8 @@ #define DM_MSG_PREFIX "persistent snapshot" #define DM_CHUNK_SIZE_DEFAULT_SECTORS 32 /* 16KB */ +#define DM_PREFETCH_CHUNKS 12 + /*----------------------------------------------------------------- * Persistent snapshots, by persistent we mean that the snapshot * will survive a reboot. @@ -490,6 +492,30 @@ static int insert_exceptions(struct pstore *ps, void *ps_area, return 0; } +static void bufio_prefetch_chunks(struct dm_bufio_client *client, + struct pstore *ps) +{ + chunk_t prefetch_area = 0; + chunk_t pf_chunk; + + if (!DM_PREFETCH_CHUNKS) + return; + + if (prefetch_area < ps->current_area) + prefetch_area = ps->current_area; + + do { + pf_chunk = area_location(ps, prefetch_area); + if (unlikely(pf_chunk >= dm_bufio_get_device_size(client))) + break; + if (unlikely(!dm_bufio_prefetch(client, pf_chunk, 1))) + break; + prefetch_area++; + if (unlikely(!prefetch_area)) + break; + } while (prefetch_area <= ps->current_area + DM_PREFETCH_CHUNKS); +} + static int read_exceptions(struct pstore *ps, int (*callback)(void *callback_context, chunk_t old, chunk_t new), @@ -505,6 +531,8 @@ static int read_exceptions(struct pstore *ps, if (IS_ERR(client)) return PTR_ERR(client); + dm_bufio_set_minimum_buffers(client, DM_PREFETCH_CHUNKS + 1); + /* * Keeping reading chunks and inserting exceptions until * we find a partially full area. @@ -512,7 +540,11 @@ static int read_exceptions(struct pstore *ps, for (ps->current_area = 0; full; ps->current_area++) { struct dm_buffer *bp; void *area; - chunk_t chunk = area_location(ps, ps->current_area); + chunk_t chunk; + + bufio_prefetch_chunks(client, ps); + + chunk = area_location(ps, ps->current_area); area = dm_bufio_read(client, chunk, &bp); if (unlikely(IS_ERR(area))) {