[PATCHES] convert dm-thin to use dm-bufio

[PATCHES] convert dm-thin to use dm-bufio

[Mikulas Patocka]

Hi

I created patches that convert dm-thin to use dm-bufio interface (so that 
it is consistent with dm-zeroed and dm-multisnap).

The patches are here:
http://people.redhat.com/mpatocka/patches/kernel/dm-thinp-bufio/

Advantages:

* It uses kmalloc, __get_free_pages or __vmalloc, so the block size is not 
limited and memory fragmentation is not an issue. The original code only 
used kmalloc.

* It has dynamic cache sizing, cache size can be configured by the user. 
In case of inactivity over some time (a buffer is unused for a minute), 
the buffers are freed. The original code had cache size hardcoded and it 
couldn't be changed.

* Submit bios directly (if we can) without dm-io layer.

Notes:

* Buffer locking is not supported, I suppose it is not used for anything 
anyway. If it is used, tell me, I can add it after reviewing it.

* dm_bm_rebind_block_device changes the block device, but it is not used 
for anything (dm-thinp never changes the device). I think it could be 
removed.

* Two small bugs in dm-thin are fixed --- not closing the superblock 
buffer on exit and improper termination sequence (the block devices were 
closed before the buffer interface exited).

Mikulas

Re: [PATCHES] convert dm-thin to use dm-bufio

[Joe Thornber]

On Sun, Aug 14, 2011 at 04:18:29PM -0400, Mikulas Patocka wrote:
> * It has dynamic cache sizing, cache size can be configured by the user. 
> In case of inactivity over some time (a buffer is unused for a minute), 
> the buffers are freed. The original code had cache size hardcoded and it 
> couldn't be changed.

A big advantage, something that would have to be done for dm-block-manager.c.
 
> * Submit bios directly (if we can) without dm-io layer.

Also good.

> Notes:
> 
> * Buffer locking is not supported, I suppose it is not used for anything 
> anyway. If it is used, tell me, I can add it after reviewing it.

Locking is used throughout the btree code, and the persistent-data
library in general.  The only thing stopping us getting the benefit
specifically in thinp is the big rw lock in dm_thin_metadata.c.  As
the code matures I plan to relax this lock to allow one writer,
multiple readers.  We've spoken about this before.

Aside from that the locking is great for debugging.  I'd have it for
that alone, and consider turning it off for release builds.

So, I'm not going to back down on the locking.  If we merge
block-manager/bufio we need locking in the interface.

> * dm_bm_rebind_block_device changes the block device, but it is not used 
> for anything (dm-thinp never changes the device). I think it could be 
> removed.

I admit it's ugly, but it _is_ used.  The need comes about from the
metadata device's lifetime being longer than that of the dm table that
opens it.  So when a different pool table is loaded we reopen the
metadata device and 'rebind' it under the block-manager.

> * Two small bugs in dm-thin are fixed --- not closing the superblock 
> buffer on exit and improper termination sequence (the block devices were 
> closed before the buffer interface exited).

Thx, I'll grab these.


This comment in the bufio header worries me:

+ * WARNING: to avoid deadlocks, the thread can hold at most one buffer. Multiple
+ * threads can hold each one buffer simultaneously.

There are many cases where persistent-data holds 2 locks, (eg, rolling
locks as we update the spine of a btree).  Also the superblock is
currently held while transactions are open (we can easily change
this).  Is this limitation easy to get round?


- Joe

Re: [PATCHES] convert dm-thin to use dm-bufio

[Mikulas Patocka]

> > Notes:
> > 
> > * Buffer locking is not supported, I suppose it is not used for anything 
> > anyway. If it is used, tell me, I can add it after reviewing it.
> 
> Locking is used throughout the btree code, and the persistent-data
> library in general.  The only thing stopping us getting the benefit
> specifically in thinp is the big rw lock in dm_thin_metadata.c.  As
> the code matures I plan to relax this lock to allow one writer,
> multiple readers.  We've spoken about this before.

If you want to relax this locking, you need:

* Lock the root pointer specially

Thread 1 is splitting the root node and thread 2 is grabbing a read lock 
on the root node. Then, when thread 1 unlocks the node, thread 2 locks the 
node, but the node is no longer a root, so thread 2 sees only part of the 
tree.

Also, 64-bit numbers are not atomic on 32-bit architectures, thus if one 
thread is splitting the root node and another thread is reading the root 
block number, it can read garbage.

* Don't allow concurrent reads while you commit

* Don't allow concurrent reads when you delete something from the tree

* Don't allow concurrent reads when you increase reference counts (i.e. 
cloning existing devices)

> Aside from that the locking is great for debugging.  I'd have it for
> that alone, and consider turning it off for release builds.
> 
> So, I'm not going to back down on the locking.  If we merge
> block-manager/bufio we need locking in the interface.
> 
> > * dm_bm_rebind_block_device changes the block device, but it is not used 
> > for anything (dm-thinp never changes the device). I think it could be 
> > removed.
> 
> I admit it's ugly, but it _is_ used.  The need comes about from the
> metadata device's lifetime being longer than that of the dm table that
> opens it.  So when a different pool table is loaded we reopen the
> metadata device and 'rebind' it under the block-manager.

BTW. what about this --- if function "pool_find" found an existing pool 
based on equivalent "metadata_dev", instead of "pool_md" as it does now. I 
think it would solve the rebind problem.

> > * Two small bugs in dm-thin are fixed --- not closing the superblock 
> > buffer on exit and improper termination sequence (the block devices were 
> > closed before the buffer interface exited).
> 
> Thx, I'll grab these.
> 
> 
> This comment in the bufio header worries me:
> 
> + * WARNING: to avoid deadlocks, the thread can hold at most one buffer. Multiple
> + * threads can hold each one buffer simultaneously.
> 
> There are many cases where persistent-data holds 2 locks, (eg, rolling
> locks as we update the spine of a btree).  Also the superblock is
> currently held while transactions are open (we can easily change
> this).  Is this limitation easy to get round?

That comment is outdated, it currently supports several buffers. But only 
one thread may hold multiple buffers (except threads that do only 
dm_bm_read_try_lock).

Mikulas

> - Joe
> 

Re: [PATCHES] convert dm-thin to use dm-bufio

[Joe Thornber]

On Mon, Aug 15, 2011 at 02:26:16PM -0400, Mikulas Patocka wrote:
> * Lock the root pointer specially
> 
> Thread 1 is splitting the root node and thread 2 is grabbing a read lock 
> on the root node. Then, when thread 1 unlocks the node, thread 2 locks the 
> node, but the node is no longer a root, so thread 2 sees only part of the 
> tree.

I'm not sure if you're talking about the superblock for the thin
metadata, or the root of any btree.

The lock I was talking about relaxing is the top level rw semaphore in
the the thin-metadata.  The locks on individual _blocks_ will still be
exclusive for writes.  So you can have concurrent lookups and inserts
happening in the btree, but the rolling locks scheme prevents races
(for instance the lookup overtaking the insert and accessing partially
written data).  This is a standard technique for copy-on-write btrees.
 
> Also, 64-bit numbers are not atomic on 32-bit architectures, thus if one 
> thread is splitting the root node and another thread is reading the root 
> block number, it can read garbage.
> 
> * Don't allow concurrent reads while you commit
> 
> * Don't allow concurrent reads when you delete something from the tree
> 
> * Don't allow concurrent reads when you increase reference counts (i.e. 
> cloning existing devices)

See answer above, I wasn't suggesting allowing reading and writing to
the same block concurrently.  So concurrent read with delete or
increasing ref counts is fine.

Concurrent reads with commit is more complicated, but not for the
reasons you give.  In this case we can let the reads run in parallel
with the commit, but we have to be careful to ensure they've completed
before we begin the new transaction to remove the possibility of recycling
one of the blocks that the read is accessing.

> > I admit it's ugly, but it _is_ used.  The need comes about from the
> > metadata device's lifetime being longer than that of the dm table that
> > opens it.  So when a different pool table is loaded we reopen the
> > metadata device and 'rebind' it under the block-manager.
> 
> BTW. what about this --- if function "pool_find" found an existing pool 
> based on equivalent "metadata_dev", instead of "pool_md" as it does now. I 
> think it would solve the rebind problem.

Originally I did bind on the metadata dev - this was the reason for
the extra metadata dev parameter in the thin target lines which was
used as the key to find the pool.

I don't think it gets round the problem that the metadata dev is
initially opened by a pool target that cannot be destroyed (or hence
reloaded) until all the devs it's opened have been closed.  So about a
month ago I was opening the metadata dev directly, rather than using
the dm_open_device thingy, which meant it could hang around longer
than the table.  I do however think the way it is now is cleaner,
because we don't duplicate dm_open_device.

> > This comment in the bufio header worries me:
> > 
> > + * WARNING: to avoid deadlocks, the thread can hold at most one buffer. Multiple
> > + * threads can hold each one buffer simultaneously.
> > 
> That comment is outdated, it currently supports several buffers. But only 
> one thread may hold multiple buffers (except threads that do only 
> dm_bm_read_try_lock).

That sounds perfect (why didn't you say this on the call yesterday
when asked about this?)

For me the main attraction to switching to bufio is you don't use
dm-io.c.

Hopefully Alasdair will allow you to keep the different memory types;
I was allocating the buffers in bm either via kmalloc, or pages
depending on size, but he wanted this changing to a kmem_cache in
order to 'segregate' the memory.

I'm not clear how you ascertain the correct working set size for your
cache, or choose when and how many blocks to flush.  Have you got any
documentation for this?  thinp shouldn't need a big cache and I'm a
bit worried a 'grab another buffer if there's memory available' policy
will make it grow too much.

- Joe

Re: [PATCHES] convert dm-thin to use dm-bufio

[Mikulas Patocka]

> For me the main attraction to switching to bufio is you don't use
> dm-io.c.
> 
> Hopefully Alasdair will allow you to keep the different memory types;
> I was allocating the buffers in bm either via kmalloc, or pages
> depending on size, but he wanted this changing to a kmem_cache in
> order to 'segregate' the memory.
> 
> I'm not clear how you ascertain the correct working set size for your
> cache, or choose when and how many blocks to flush.  Have you got any
> documentation for this?  thinp shouldn't need a big cache and I'm a
> bit worried a 'grab another buffer if there's memory available' policy
> will make it grow too much.

The working set is 2% of memory or 1/4 of vmalloc space (whichever is 
smaller). This can be changed in 
"/sys/module/dm_bufio/parameters/dm_bufio_cache_size".

Buffers are freed when they are unused for 60 seconds (can be changed in 
"/sys/module/dm_bufio/parameters/dm_bufio_max_age").

Total size is divided by the number of active clients using dm-bufio. So 
that each client gets a fair share.

Writeback is done on commit. When 3/4 of the cache size is used, 
background writeback is started.

Mikulas

> - Joe
> 

Re: [PATCHES] convert dm-thin to use dm-bufio

[Joe Thornber]

On Tue, Aug 16, 2011 at 06:03:00PM -0400, Mikulas Patocka wrote:
> The working set is 2% of memory or 1/4 of vmalloc space (whichever is 
> smaller). This can be changed in 
> "/sys/module/dm_bufio/parameters/dm_bufio_cache_size".
> 
> Buffers are freed when they are unused for 60 seconds (can be changed in 
> "/sys/module/dm_bufio/parameters/dm_bufio_max_age").
> 
> Total size is divided by the number of active clients using dm-bufio. So 
> that each client gets a fair share.
> 
> Writeback is done on commit. When 3/4 of the cache size is used, 
> background writeback is started.

This all sounds good; get the locking interface in and I'll switch to
bufio straight away.

- Joe

Re: [PATCHES] convert dm-thin to use dm-bufio

[Mikulas Patocka]

> This all sounds good; get the locking interface in and I'll switch to
> bufio straight away.
> 
> - Joe

I uploaded bufio-based block manager at 
http://people.redhat.com/mpatocka/patches/kernel/dm-thinp-bufio/. It 
supports locks, but it defines new functions down_write_non_owner and 
up_write_non_owner.

Mikulas

Re: convert dm-thin to use dm-bufio

[Mike Snitzer]

On Thu, Aug 18 2011 at  6:31pm -0400,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> > This all sounds good; get the locking interface in and I'll switch to
> > bufio straight away.
> > 
> > - Joe
> 
> I uploaded bufio-based block manager at 
> http://people.redhat.com/mpatocka/patches/kernel/dm-thinp-bufio/. It 
> supports locks, but it defines new functions down_write_non_owner and 
> up_write_non_owner.

dm-bufio.patch:
drivers/md/Kconfig needs a more comprehensive description for DM_BUFIO's
help.


dm-thinp-bufio.patch:
1)
This drivers/md/persistent-data/dm-block-manager.h change avoids lots of
block manager interface churn:

-struct dm_block;
+#define dm_block		dm_buffer
+#define dm_block_manager	dm_bufio_client

But I think it'd be best, in the long run, to have a follow-on patch
that does away with the aliases and just use the bufio structs
throughout the code.  Anyway, don't need to worry about this now.  But
what you've done is hack that should probably be cleaned up.

2)
dm_bm_rebind_block_device:
+	 * !!! FIXME: remove this. It is supposedly unused.

I'll need to look closer at the overall changes to dm-block-manager.c
but you've clearly gotten rid of struct dm_block_manager; so there is no
reference to a bdev that needs to be flushed.

Which begs the question:
Have you tested this bufio conversion with the thinp-test-suite?
git://github.com/jthornber/thinp-test-suite.git




Question for Joe:
You're making conflicting changes quick enough that I wonder if you
and Mikulas will ever converge (e.g. why do multiple block managers need
to have access to the same metadata device!?).

-  - [ ] Sometimes I'm getting kmem_cache name clashes in the block
+  - [X] Sometimes I'm getting kmem_cache name clashes in the block
         manager.  We're obviously not deleting the cache in a particular
         error path.  Recently introduced.                               :ejt:

commit 049bf17f41147ba3d51bac6ebee038d3d79a086c
Author: Joe Thornber <ejt@redhat.com>
Date:   Wed Aug 17 13:46:12 2011 +0100

    [block-manager, thin-metadata] Make the bm kmemcache name unique to
    the pool.
    
    ATM the kmem_cache identifier just includes the major/minor of the
    device that the bm is looking at.  This means you can't create 2 bms
    that point to the same device without generating a nasty stack
    trace.
    
    This patch makes the name unique to the pool.  It's a quick hack to
    allow me to keep on testing.  Could someone who cares about these
    kmemcaches decide on a proper solution please.

Re: convert dm-thin to use dm-bufio

[Joe Thornber]

On Fri, Aug 19, 2011 at 03:04:36AM -0400, Mike Snitzer wrote:
> Question for Joe:
> You're making conflicting changes quick enough that I wonder if you
> and Mikulas will ever converge (e.g. why do multiple block managers need
> to have access to the same metadata device!?).

They don't; my issue is with getting an oops if they do through user
error.  I clearly said in the commit message that this was a hack to
get round issues introduced by agk's move to a kmemcache.  Cook
something cleaner up between yourselves, or wait for me to look at it
again once I've got through some more pressing issues.

An alternative would be to iterate through all the pools in the system
check whether any of them already had the same metadata device open.
Of course this doesn't catch the cases where the stacking is used and
a metadata device eventually maps onto the same physical disk as an
existing md area.

- Joe

Re: [PATCHES] convert dm-thin to use dm-bufio

[Joe Thornber]

On Thu, Aug 18, 2011 at 06:31:13PM -0400, Mikulas Patocka wrote:
> I uploaded bufio-based block manager at 
> http://people.redhat.com/mpatocka/patches/kernel/dm-thinp-bufio/. It 
> supports locks, but it defines new functions down_write_non_owner and 
> up_write_non_owner.

Thanks Mikulas, I'll try and get it merged over the next couple of days.

- Joe

Re: convert dm-thin to use dm-bufio

[Mike Snitzer]

On Fri, Aug 19 2011 at  5:11am -0400,
Joe Thornber <thornber@redhat.com> wrote:

> On Fri, Aug 19, 2011 at 03:04:36AM -0400, Mike Snitzer wrote:
> > Question for Joe:
> > You're making conflicting changes quick enough that I wonder if you
> > and Mikulas will ever converge (e.g. why do multiple block managers need
> > to have access to the same metadata device!?).
> 
> They don't; my issue is with getting an oops if they do through user
> error.  I clearly said in the commit message that this was a hack to
> get round issues introduced by agk's move to a kmemcache.  Cook
> something cleaner up between yourselves, or wait for me to look at it
> again once I've got through some more pressing issues.

OK, so this kmemcache problem will go away once you switch over to
bufio.
 
> An alternative would be to iterate through all the pools in the system
> check whether any of them already had the same metadata device open.
> Of course this doesn't catch the cases where the stacking is used and
> a metadata device eventually maps onto the same physical disk as an
> existing md area.

The lack of checking for a metadata or data device that is already in
use should probably be fixed.

As for stacking, can't we just read the superblock to check if a device
is already in use (would work metadata anyway)?  No idea if that'd be
too costly -- probably not: read superblock and check if
THIN_SUPERBLOCK_MAGIC is set.

Re: convert dm-thin to use dm-bufio

[Mike Snitzer]

On Fri, Aug 19 2011 at  5:46am -0400,
Mike Snitzer <snitzer@redhat.com> wrote:

> On Fri, Aug 19 2011 at  5:11am -0400,
> Joe Thornber <thornber@redhat.com> wrote:
>
> > An alternative would be to iterate through all the pools in the system
> > check whether any of them already had the same metadata device open.
> > Of course this doesn't catch the cases where the stacking is used and
> > a metadata device eventually maps onto the same physical disk as an
> > existing md area.
> 
> The lack of checking for a metadata or data device that is already in
> use should probably be fixed.
> 
> As for stacking, can't we just read the superblock to check if a device
> is already in use (would work metadata anyway)?  No idea if that'd be
> too costly -- probably not: read superblock and check if
> THIN_SUPERBLOCK_MAGIC is set.

Bleh, that won't work.  THIN_SUPERBLOCK_MAGIC is the same for all
devices...

dm_pool_metadata_open() knows when a superblock isn't all zeroes.  We
could read the suerblock's uuid (which we provisioned for in the
superblock but don't yet set) and then go searching all other pools to
see if the same superblock uuid exists.

Re: convert dm-thin to use dm-bufio

[Joe Thornber]

On Fri, Aug 19, 2011 at 05:46:04AM -0400, Mike Snitzer wrote:
> On Fri, Aug 19 2011 at  5:11am -0400,
> Joe Thornber <thornber@redhat.com> wrote:
> 
> > On Fri, Aug 19, 2011 at 03:04:36AM -0400, Mike Snitzer wrote:
> > > Question for Joe:
> > > You're making conflicting changes quick enough that I wonder if you
> > > and Mikulas will ever converge (e.g. why do multiple block managers need
> > > to have access to the same metadata device!?).
> > 
> > They don't; my issue is with getting an oops if they do through user
> > error.  I clearly said in the commit message that this was a hack to
> > get round issues introduced by agk's move to a kmemcache.  Cook
> > something cleaner up between yourselves, or wait for me to look at it
> > again once I've got through some more pressing issues.
> 
> OK, so this kmemcache problem will go away once you switch over to
> bufio.

Assuming agk doesn't decide to switch over to a kmemcache.  Remember
bm used to allocate via kmalloc or pages like bufio.

> As for stacking, can't we just read the superblock to check if a device
> is already in use (would work metadata anyway)?  No idea if that'd be
> too costly -- probably not: read superblock and check if
> THIN_SUPERBLOCK_MAGIC is set.

So you've already thought of a scenario where it's worth having two bm
instances (one read only).

- Joe

Re: convert dm-thin to use dm-bufio

[Mikulas Patocka]

On Fri, 19 Aug 2011, Mike Snitzer wrote:

> On Thu, Aug 18 2011 at  6:31pm -0400,
> Mikulas Patocka <mpatocka@redhat.com> wrote:
> 
> > > This all sounds good; get the locking interface in and I'll switch to
> > > bufio straight away.
> > > 
> > > - Joe
> > 
> > I uploaded bufio-based block manager at 
> > http://people.redhat.com/mpatocka/patches/kernel/dm-thinp-bufio/. It 
> > supports locks, but it defines new functions down_write_non_owner and 
> > up_write_non_owner.
> 
> dm-bufio.patch:
> drivers/md/Kconfig needs a more comprehensive description for DM_BUFIO's
> help.

There is intentionally no description. If there is no description, this 
option won't appear in "make menuconfig" menu.

dm-bufio is selected automatically when some module that needs it is 
configured. There is no need to select dm-bufio manually.

> dm-thinp-bufio.patch:
> 1)
> This drivers/md/persistent-data/dm-block-manager.h change avoids lots of
> block manager interface churn:
> 
> -struct dm_block;
> +#define dm_block		dm_buffer
> +#define dm_block_manager	dm_bufio_client
> 
> But I think it'd be best, in the long run, to have a follow-on patch
> that does away with the aliases and just use the bufio structs
> throughout the code.  Anyway, don't need to worry about this now.  But
> what you've done is hack that should probably be cleaned up.

This way, it's easy to swap the original block manager and dm-bufio.

If I changed the names throughout the whole dm-thinp code, it would 
conflict with any changes Joe may do.

Mikulas

Re: convert dm-thin to use dm-bufio

[Mike Snitzer]

On Fri, Aug 19 2011 at  6:22am -0400,
Joe Thornber <thornber@redhat.com> wrote:

> On Fri, Aug 19, 2011 at 05:46:04AM -0400, Mike Snitzer wrote:
> > On Fri, Aug 19 2011 at  5:11am -0400,
> > Joe Thornber <thornber@redhat.com> wrote:
> > 
> > > On Fri, Aug 19, 2011 at 03:04:36AM -0400, Mike Snitzer wrote:
> > > > Question for Joe:
> > > > You're making conflicting changes quick enough that I wonder if you
> > > > and Mikulas will ever converge (e.g. why do multiple block managers need
> > > > to have access to the same metadata device!?).
> > > 
> > > They don't; my issue is with getting an oops if they do through user
> > > error.  I clearly said in the commit message that this was a hack to
> > > get round issues introduced by agk's move to a kmemcache.  Cook
> > > something cleaner up between yourselves, or wait for me to look at it
> > > again once I've got through some more pressing issues.
> > 
> > OK, so this kmemcache problem will go away once you switch over to
> > bufio.
> 
> Assuming agk doesn't decide to switch over to a kmemcache.  Remember
> bm used to allocate via kmalloc or pages like bufio.

Indeed.
 
> > As for stacking, can't we just read the superblock to check if a device
> > is already in use (would work metadata anyway)?  No idea if that'd be
> > too costly -- probably not: read superblock and check if
> > THIN_SUPERBLOCK_MAGIC is set.
> 
> So you've already thought of a scenario where it's worth having two bm
> instances (one read only).

I didn't realize it but excellent point.

Re: convert dm-thin to use dm-bufio

[Alasdair G Kergon]

On Fri, Aug 19, 2011 at 09:49:21AM -0400, Mike Snitzer wrote:
> On Fri, Aug 19 2011 at  6:22am -0400,
> Joe Thornber <thornber@redhat.com> wrote:
> > Assuming agk doesn't decide to switch over to a kmemcache.  Remember
> > bm used to allocate via kmalloc or pages like bufio.
> Indeed.
  
The basic requirement is to know accurately how much memory is being used
by the module (and ultimately per-pool device) as I can imagine cases 
where it's using a significant proportion of the system's memory.

I doubt kmemcache would be a satisfactory solution for bufio, but there
are other ways to meet this requirement, such as exporting this
information in /sys.  Initally just a global figure for the module, but
evenutally reported per-device (with dm-core support and new target
callbacks).

Alasdair

Re: convert dm-thin to use dm-bufio

[Mike Snitzer]

On Fri, Aug 19 2011 at  9:31am -0400,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> On Fri, 19 Aug 2011, Mike Snitzer wrote:
> 
> > On Thu, Aug 18 2011 at  6:31pm -0400,
> > Mikulas Patocka <mpatocka@redhat.com> wrote:
> > 
> > > > This all sounds good; get the locking interface in and I'll switch to
> > > > bufio straight away.
> > > > 
> > > > - Joe
> > > 
> > > I uploaded bufio-based block manager at 
> > > http://people.redhat.com/mpatocka/patches/kernel/dm-thinp-bufio/. It 
> > > supports locks, but it defines new functions down_write_non_owner and 
> > > up_write_non_owner.
> > 
> > dm-bufio.patch:
> > drivers/md/Kconfig needs a more comprehensive description for DM_BUFIO's
> > help.
> 
> There is intentionally no description. If there is no description, this 
> option won't appear in "make menuconfig" menu.
> 
> dm-bufio is selected automatically when some module that needs it is 
> configured. There is no need to select dm-bufio manually.

Hmm, I wonder if it makes sense to remove DM_PERSISTENT_DATA's help
description?

(we already select DM_PERSISTENT_DATA when DM_THIN_PROVISIONING is
selected -- but it is visible in menuconfig because it has a
description).

Re: convert dm-thin to use dm-bufio

[Joe Thornber]

On Fri, Aug 19, 2011 at 09:31:56AM -0400, Mikulas Patocka wrote:
> On Fri, 19 Aug 2011, Mike Snitzer wrote:
> > -struct dm_block;
> > +#define dm_block		dm_buffer
> > +#define dm_block_manager	dm_bufio_client
> > 
> > But I think it'd be best, in the long run, to have a follow-on patch
> > that does away with the aliases and just use the bufio structs
> > throughout the code.  Anyway, don't need to worry about this now.  But
> > what you've done is hack that should probably be cleaned up.
> 
> This way, it's easy to swap the original block manager and dm-bufio.
> 
> If I changed the names throughout the whole dm-thinp code, it would 
> conflict with any changes Joe may do.

I agree with Mikulas here.

Re: [PATCHES] convert dm-thin to use dm-bufio

[Joe Thornber]

Mikulas,

On Fri, Aug 19, 2011 at 10:12:24AM +0100, Joe Thornber wrote:
> On Thu, Aug 18, 2011 at 06:31:13PM -0400, Mikulas Patocka wrote:
> > I uploaded bufio-based block manager at 
> > http://people.redhat.com/mpatocka/patches/kernel/dm-thinp-bufio/. It 
> > supports locks, but it defines new functions down_write_non_owner and 
> > up_write_non_owner.
> 
> Thanks Mikulas, I'll try and get it merged over the next couple of days.

I've merged and pushed this to my git repo.

All the test-suite is passing.  Performance is identical (though
hopefully we're a lot more scaleable now).

I have to say I think the separation of io and locking is very elegant
now.  Well done.

I've made two small changes:

i) I switched to using dm_bufio_new() rather than dm_bufio_read() at
the start of *write_lock_zero().  No point reading the data if we're
going to throw it away.

ii) We no longer hold the superblock lock for the duration of the
transaction.  This means we can get rid of the nasty rwsem non-owner
patch.

We have one outstanding issue however.  Now we're using a rwsem per
block, lockdep is trying to validate there are no deadlocks.  Which it
can't do, so is producing a warning. See the
   
 "Exception: Nested data dependencies leading to nested locking"

section in lockdep-design.txt for more discussion of the problem.

We need to suppress this somehow.  I guess the right thing to do is
use the nesting level facility, eg call the superblock level 0, btree
roots level 1 etc.  This will involve a lot of code changes however.
So for now I just want to turn off checking on those locks.

- Joe

Re: convert dm-thin to use dm-bufio

[Mike Snitzer]

On Fri, Aug 19 2011 at 11:37am -0400,
Joe Thornber <thornber@redhat.com> wrote:

> On Fri, Aug 19, 2011 at 09:31:56AM -0400, Mikulas Patocka wrote:
> > On Fri, 19 Aug 2011, Mike Snitzer wrote:
> > > -struct dm_block;
> > > +#define dm_block		dm_buffer
> > > +#define dm_block_manager	dm_bufio_client
> > > 
> > > But I think it'd be best, in the long run, to have a follow-on patch
> > > that does away with the aliases and just use the bufio structs
> > > throughout the code.  Anyway, don't need to worry about this now.  But
> > > what you've done is hack that should probably be cleaned up.
> > 
> > This way, it's easy to swap the original block manager and dm-bufio.
> > 
> > If I changed the names throughout the whole dm-thinp code, it would 
> > conflict with any changes Joe may do.
> 
> I agree with Mikulas here.

It could be that we're all in agreement here.

In the near term it makes no sense to have the structure name churn
given the merge of bufio will be disruptive enough as it is.

I tried to convey as much above with: "Anyway, don't need to worry about
this now."

But if thinp is to be the first consumer of bufio (a substantial chunk
of code in its own right) then it stands to reason we should have thinp
act as the reference for how other targets should consume it.

So all I was suggesting is that once thinp is converted to bufio, and
there is confidence in the result, it should be updated to not have the
intermediate structure aliases that helped ease the conversion.

If what you're saying is thinp should always use the structure aliases
then I guess we'll just disagree and that'll be the end of it.  All I
can do is say my peace.

Re: convert dm-thin to use dm-bufio

[Alasdair G Kergon]

On Fri, Aug 19, 2011 at 02:25:32PM -0400, Mike Snitzer wrote:
> It could be that we're all in agreement here.
 
It's an easy change - whichever set of names we choose - and it can be done at
any time before the switch, if we go ahead with it, is ready for linux-next,
but there's no urgency to change it yet while both versions are in use.

Alasdair

[PATCH 1/2] dm bufio: fix "value computed is not used" warnings

[Mike Snitzer]

---
 drivers/md/dm-bufio.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/md/dm-bufio.c b/drivers/md/dm-bufio.c
index 942dc59..4ecb8b8 100644
--- a/drivers/md/dm-bufio.c
+++ b/drivers/md/dm-bufio.c
@@ -213,7 +213,7 @@ static void cache_size_refresh(void)
 		 * Modify dm_bufio_cache_size to report the real used cache
 		 * size to the user.
 		 */
-		cmpxchg(&dm_bufio_cache_size, 0, dm_bufio_default_cache_size);
+		(void)cmpxchg(&dm_bufio_cache_size, 0, dm_bufio_default_cache_size);
 		dm_bufio_cache_size_latch = dm_bufio_default_cache_size;
 	}
 	dm_bufio_cache_size_per_client = dm_bufio_cache_size_latch /
@@ -841,7 +841,7 @@ static void write_endio(struct bio *bio, int error)
 	b->write_error = error;
 	if (unlikely(error)) {
 		struct dm_bufio_client *c = b->c;
-		cmpxchg(&c->async_write_error, 0, error);
+		(void)cmpxchg(&c->async_write_error, 0, error);
 	}
 	BUG_ON(!test_bit(B_WRITING, &b->state));
 	smp_mb__before_clear_bit();
-- 
1.7.4.4

[PATCH 2/2] dm space map: only include bitops.h in dm-space-map-common.c

[Mike Snitzer]

---
 drivers/md/persistent-data/dm-space-map-common.c   |    1 +
 drivers/md/persistent-data/dm-space-map-disk.c     |    1 -
 drivers/md/persistent-data/dm-space-map-metadata.c |    1 -
 3 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/drivers/md/persistent-data/dm-space-map-common.c b/drivers/md/persistent-data/dm-space-map-common.c
index 90cc199..513ef86 100644
--- a/drivers/md/persistent-data/dm-space-map-common.c
+++ b/drivers/md/persistent-data/dm-space-map-common.c
@@ -1,6 +1,7 @@
 #include "dm-space-map-common.h"
 #include "dm-transaction-manager.h"
 
+#include <linux/bitops.h>
 #include <linux/device-mapper.h>
 
 #define DM_MSG_PREFIX "space map common"
diff --git a/drivers/md/persistent-data/dm-space-map-disk.c b/drivers/md/persistent-data/dm-space-map-disk.c
index ba917cd..dee112f 100644
--- a/drivers/md/persistent-data/dm-space-map-disk.c
+++ b/drivers/md/persistent-data/dm-space-map-disk.c
@@ -11,7 +11,6 @@
 
 #include <linux/list.h>
 #include <linux/slab.h>
-#include <linux/bitops.h>
 #include <linux/module.h>
 #include <linux/device-mapper.h>
 
diff --git a/drivers/md/persistent-data/dm-space-map-metadata.c b/drivers/md/persistent-data/dm-space-map-metadata.c
index 49adf0d..1911c41 100644
--- a/drivers/md/persistent-data/dm-space-map-metadata.c
+++ b/drivers/md/persistent-data/dm-space-map-metadata.c
@@ -10,7 +10,6 @@
 
 #include <linux/list.h>
 #include <linux/slab.h>
-#include <linux/bitops.h>
 #include <linux/device-mapper.h>
 
 #define DM_MSG_PREFIX "space map metadata"
-- 
1.7.4.4

Re: [PATCH 1/2] dm bufio: fix "value computed is not used" warnings

[Mikulas Patocka]

OK, I applied the patch.

Mikulas

Re: [PATCHES] convert dm-thin to use dm-bufio

[Mikulas Patocka]

> Mikulas,
> 
> On Fri, Aug 19, 2011 at 10:12:24AM +0100, Joe Thornber wrote:
> > On Thu, Aug 18, 2011 at 06:31:13PM -0400, Mikulas Patocka wrote:
> > > I uploaded bufio-based block manager at 
> > > http://people.redhat.com/mpatocka/patches/kernel/dm-thinp-bufio/. It 
> > > supports locks, but it defines new functions down_write_non_owner and 
> > > up_write_non_owner.
> > 
> > Thanks Mikulas, I'll try and get it merged over the next couple of days.
> 
> I've merged and pushed this to my git repo.

I checkouted the tree now (I tried all branches reported with git-branch 
-a), but I couldn't see bufio in any of the branches.

> All the test-suite is passing.  Performance is identical (though
> hopefully we're a lot more scaleable now).
> 
> I have to say I think the separation of io and locking is very elegant
> now.  Well done.
> 
> I've made two small changes:
> 
> i) I switched to using dm_bufio_new() rather than dm_bufio_read() at
> the start of *write_lock_zero().  No point reading the data if we're
> going to throw it away.

OK, I forgot about this.

> ii) We no longer hold the superblock lock for the duration of the
> transaction.  This means we can get rid of the nasty rwsem non-owner
> patch.
> 
> We have one outstanding issue however.  Now we're using a rwsem per
> block, lockdep is trying to validate there are no deadlocks.  Which it
> can't do, so is producing a warning. See the
>    
>  "Exception: Nested data dependencies leading to nested locking"
> 
> section in lockdep-design.txt for more discussion of the problem.
> 
> We need to suppress this somehow.  I guess the right thing to do is
> use the nesting level facility, eg call the superblock level 0, btree
> roots level 1 etc.  This will involve a lot of code changes however.
> So for now I just want to turn off checking on those locks.

I think you can't turn off lockdep just so. Those "non-owner" lock 
versions, bypass lockdep.

Mikulas

> - Joe
> 

Re: [PATCHES] convert dm-thin to use dm-bufio

[Mikulas Patocka]

> > I've merged and pushed this to my git repo.
> 
> I checkouted the tree now (I tried all branches reported with git-branch 
> -a), but I couldn't see bufio in any of the branches.

It was my mistake --- I forgot to do git-fetch before git-checkout.

Here is a patch that uses down_read_trylock instead of down_read in 
dm_bm_read_try_lock.

Mikulas

---
 drivers/md/persistent-data/dm-block-manager.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Index: linux-3.0-fast/drivers/md/persistent-data/dm-block-manager.c
===================================================================
--- linux-3.0-fast.orig/drivers/md/persistent-data/dm-block-manager.c	2011-08-22 21:53:31.000000000 +0200
+++ linux-3.0-fast/drivers/md/persistent-data/dm-block-manager.c	2011-08-22 21:54:38.000000000 +0200
@@ -176,7 +176,10 @@ int dm_bm_read_try_lock(struct dm_block_
 		return -EWOULDBLOCK;
 
 	aux = dm_bufio_get_aux_data(*result);
-	down_read(&aux->lock);
+	if (unlikely(!down_read_trylock(&aux->lock))) {
+		dm_bufio_release(*result);
+		return -EWOULDBLOCK;
+	}
 	aux->write_locked = 0;
 
 	r = dm_bm_validate_buffer(bm, *result, aux, v);

Re: [PATCHES] convert dm-thin to use dm-bufio

[Joe Thornber]

On Mon, Aug 22, 2011 at 03:59:05PM -0400, Mikulas Patocka wrote:
> Here is a patch that uses down_read_trylock instead of down_read in 
> dm_bm_read_try_lock.

pushed, thanks.