Re: [PATCH 0/4] Fix a crash when block device is read and block size is changed at the same time

[Mikulas Patocka <mpatocka@redhat.com>]

On Fri, 31 Aug 2012, Jeff Moyer wrote:

> Mikulas Patocka <mpatocka@redhat.com> writes:
> 
> > On Fri, 31 Aug 2012, Mikulas Patocka wrote:
> >
> >> Hi
> >> 
> >> This is a series of patches to prevent a crash when when someone is 
> >> reading block device and block size is changed simultaneously. (the crash 
> >> is already happening in the production environment)
> >> 
> >> The first patch adds a rw-lock to struct block_device, but doesn't use the 
> >> lock anywhere. The reason why I submit this as a separate patch is that on 
> >> my computer adding an unused field to this structure affects performance 
> >> much more than any locking changes.
> >> 
> >> The second patch uses the rw-lock. The lock is locked for read when doing 
> >> I/O on the block device and it is locked for write when changing block 
> >> size.
> >> 
> >> The third patch converts the rw-lock to a percpu rw-lock for better 
> >> performance, to avoid cache line bouncing.
> >> 
> >> The fourth patch is an alternate percpu rw-lock implementation using RCU 
> >> by Eric Dumazet. It avoids any atomic instruction in the hot path.
> >> 
> >> Mikulas
> >
> > I tested performance of patches. I created 4GB ramdisk, I initially filled 
> > it with zeros (so that ramdisk allocation-on-demand doesn't affect the 
> > results).
> >
> > I ran fio to perform 8 concurrent accesses on 8 core machine (two 
> > Barcelona Opterons):
> > time fio --rw=randrw --size=4G --bs=512 --filename=/dev/ram0 --direct=1 
> > --name=job1 --name=job2 --name=job3 --name=job4 --name=job5 --name=job6 
> > --name=job7 --name=job8
> >
> > The results actually show that the size of struct block_device and 
> > alignment of subsequent fields in struct inode have far more effect on 
> > result that the type of locking used. (struct inode is placed just after 
> > struct block_device in "struct bdev_inode" in fs/block-dev.c)
> >
> > plain kernel 3.5.3: 57.9s
> > patch 1: 43.4s
> > patches 1,2: 43.7s
> > patches 1,2,3: 38.5s
> > patches 1,2,3,4: 58.6s
> >
> > You can see that patch 1 improves the time by 14.5 seconds, but all that 
> > patch 1 does is adding an unused structure field.
> >
> > Patch 3 is 4.9 seconds faster than patch 1, althogh patch 1 does no 
> > locking at all and patch 3 does per-cpu locking. So, the reason for the 
> > speedup is different sizeof of struct block_device (and subsequently, 
> > different alignment of struct inode), rather than locking improvement.
> 
> How many runs did you do?  Did you see much run to run variation?

These results come from two runs (which differed by no more than 1s), but 
I observed the same phenomenon - difference in time due to the size of 
block_device - many times before when I was doing benchmarking when 
developing these patches.

I actually had to apply something like this to make the results not depend 
on the size of block_dev.

I would be interested if the same difference could be observed on other 
processors or if it is something specific to AMD K10 architecture.

---
 fs/block_dev.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Index: linux-3.5.3-fast/fs/block_dev.c
===================================================================
--- linux-3.5.3-fast.orig/fs/block_dev.c	2012-08-31 22:30:07.000000000 +0200
+++ linux-3.5.3-fast/fs/block_dev.c	2012-08-31 22:30:43.000000000 +0200
@@ -31,7 +31,10 @@
 #include "internal.h"
 
 struct bdev_inode {
-	struct block_device bdev;
+	union {
+		struct block_device bdev;
+		char pad[0x140];
+	};
 	struct inode vfs_inode;
 };
 

> > I would be interested if other people did performance testing of the 
> > patches too.
> 
> I'll do some testing next week, but don't expect to get to it before
> Wednesday.
> 
> Cheers,
> Jeff

Mikulas