Re: [PATCH 2/2] block_dev: don't take the write lock if block size doesn't change
On Wed, 28 Nov 2012, Jeff Chua wrote: > On Wed, Nov 28, 2012 at 12:01 PM, Mikulas Patocka wrote: > > block_dev: don't take the write lock if block size doesn't change > > > > Taking the write lock has a big performance impact on the whole system > > (because of synchronize_sched_expedited). This patch avoids taking the > > write lock if the block size doesn't change (i.e. when mounting > > filesystem with block size equal to the default block size). > > > > The logic to test if the block device is mapped was moved to a separate > > function is_bdev_mapped to avoid code duplication. > > > > Signed-off-by: Mikulas Patocka > > > > --- > > fs/block_dev.c | 25 ++--- > > 1 file changed, 18 insertions(+), 7 deletions(-) > > > > Index: linux-3.7-rc7/fs/block_dev.c > > === > > --- linux-3.7-rc7.orig/fs/block_dev.c 2012-11-28 04:09:01.0 +0100 > > +++ linux-3.7-rc7/fs/block_dev.c2012-11-28 04:13:53.0 +0100 > > @@ -114,10 +114,18 @@ void invalidate_bdev(struct block_device > > } > > EXPORT_SYMBOL(invalidate_bdev); > > > > -int set_blocksize(struct block_device *bdev, int size) > > +static int is_bdev_mapped(struct block_device *bdev) > > { > > - struct address_space *mapping; > > + int ret_val; > > + struct address_space *mapping = bdev->bd_inode->i_mapping; > > + mutex_lock(>i_mmap_mutex); > > + ret_val = mapping_mapped(mapping); > > + mutex_unlock(>i_mmap_mutex); > > + return ret_val; > > +} > > > > +int set_blocksize(struct block_device *bdev, int size) > > +{ > > /* Size must be a power of two, and between 512 and PAGE_SIZE */ > > if (size > PAGE_SIZE || size < 512 || !is_power_of_2(size)) > > return -EINVAL; > > @@ -126,18 +134,21 @@ int set_blocksize(struct block_device *b > > if (size < bdev_logical_block_size(bdev)) > > return -EINVAL; > > > > + /* > > +* If the block size doesn't change, don't take the write lock. > > +* We check for is_bdev_mapped anyway, for consistent behavior. > > +*/ > > + if (size == bdev->bd_block_size) > > + return is_bdev_mapped(bdev) ? -EBUSY : 0; > > + > > /* Prevent starting I/O or mapping the device */ > > percpu_down_write(>bd_block_size_semaphore); > > > > /* Check that the block device is not memory mapped */ > > - mapping = bdev->bd_inode->i_mapping; > > - mutex_lock(>i_mmap_mutex); > > - if (mapping_mapped(mapping)) { > > - mutex_unlock(>i_mmap_mutex); > > + if (is_bdev_mapped(bdev)) { > > percpu_up_write(>bd_block_size_semaphore); > > return -EBUSY; > > } > > - mutex_unlock(>i_mmap_mutex); > > > > /* Don't change the size if it is same as current */ > > if (bdev->bd_block_size != size) { > > > This patch didn't really make any difference for ext2/3/4 but for > reiserfs it does. > > With the synchronize_sched_expedited() patch applied, it didn't make > any difference. > > Thanks, > Jeff It could make difference for computers with many cores - synchronize_sched_expedited() is expensive there because it synchronizes all active cores, so if we can avoid it, just do it. Mikulas -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] block_dev: don't take the write lock if block size doesn't change
On Wed, Nov 28, 2012 at 12:01 PM, Mikulas Patocka wrote: > block_dev: don't take the write lock if block size doesn't change > > Taking the write lock has a big performance impact on the whole system > (because of synchronize_sched_expedited). This patch avoids taking the > write lock if the block size doesn't change (i.e. when mounting > filesystem with block size equal to the default block size). > > The logic to test if the block device is mapped was moved to a separate > function is_bdev_mapped to avoid code duplication. > > Signed-off-by: Mikulas Patocka > > --- > fs/block_dev.c | 25 ++--- > 1 file changed, 18 insertions(+), 7 deletions(-) > > Index: linux-3.7-rc7/fs/block_dev.c > === > --- linux-3.7-rc7.orig/fs/block_dev.c 2012-11-28 04:09:01.0 +0100 > +++ linux-3.7-rc7/fs/block_dev.c2012-11-28 04:13:53.0 +0100 > @@ -114,10 +114,18 @@ void invalidate_bdev(struct block_device > } > EXPORT_SYMBOL(invalidate_bdev); > > -int set_blocksize(struct block_device *bdev, int size) > +static int is_bdev_mapped(struct block_device *bdev) > { > - struct address_space *mapping; > + int ret_val; > + struct address_space *mapping = bdev->bd_inode->i_mapping; > + mutex_lock(>i_mmap_mutex); > + ret_val = mapping_mapped(mapping); > + mutex_unlock(>i_mmap_mutex); > + return ret_val; > +} > > +int set_blocksize(struct block_device *bdev, int size) > +{ > /* Size must be a power of two, and between 512 and PAGE_SIZE */ > if (size > PAGE_SIZE || size < 512 || !is_power_of_2(size)) > return -EINVAL; > @@ -126,18 +134,21 @@ int set_blocksize(struct block_device *b > if (size < bdev_logical_block_size(bdev)) > return -EINVAL; > > + /* > +* If the block size doesn't change, don't take the write lock. > +* We check for is_bdev_mapped anyway, for consistent behavior. > +*/ > + if (size == bdev->bd_block_size) > + return is_bdev_mapped(bdev) ? -EBUSY : 0; > + > /* Prevent starting I/O or mapping the device */ > percpu_down_write(>bd_block_size_semaphore); > > /* Check that the block device is not memory mapped */ > - mapping = bdev->bd_inode->i_mapping; > - mutex_lock(>i_mmap_mutex); > - if (mapping_mapped(mapping)) { > - mutex_unlock(>i_mmap_mutex); > + if (is_bdev_mapped(bdev)) { > percpu_up_write(>bd_block_size_semaphore); > return -EBUSY; > } > - mutex_unlock(>i_mmap_mutex); > > /* Don't change the size if it is same as current */ > if (bdev->bd_block_size != size) { This patch didn't really make any difference for ext2/3/4 but for reiserfs it does. With the synchronize_sched_expedited() patch applied, it didn't make any difference. Thanks, Jeff -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] block_dev: don't take the write lock if block size doesn't change
On Wed, Nov 28, 2012 at 12:01 PM, Mikulas Patocka mpato...@redhat.com wrote: block_dev: don't take the write lock if block size doesn't change Taking the write lock has a big performance impact on the whole system (because of synchronize_sched_expedited). This patch avoids taking the write lock if the block size doesn't change (i.e. when mounting filesystem with block size equal to the default block size). The logic to test if the block device is mapped was moved to a separate function is_bdev_mapped to avoid code duplication. Signed-off-by: Mikulas Patocka mpato...@redhat.com --- fs/block_dev.c | 25 ++--- 1 file changed, 18 insertions(+), 7 deletions(-) Index: linux-3.7-rc7/fs/block_dev.c === --- linux-3.7-rc7.orig/fs/block_dev.c 2012-11-28 04:09:01.0 +0100 +++ linux-3.7-rc7/fs/block_dev.c2012-11-28 04:13:53.0 +0100 @@ -114,10 +114,18 @@ void invalidate_bdev(struct block_device } EXPORT_SYMBOL(invalidate_bdev); -int set_blocksize(struct block_device *bdev, int size) +static int is_bdev_mapped(struct block_device *bdev) { - struct address_space *mapping; + int ret_val; + struct address_space *mapping = bdev-bd_inode-i_mapping; + mutex_lock(mapping-i_mmap_mutex); + ret_val = mapping_mapped(mapping); + mutex_unlock(mapping-i_mmap_mutex); + return ret_val; +} +int set_blocksize(struct block_device *bdev, int size) +{ /* Size must be a power of two, and between 512 and PAGE_SIZE */ if (size PAGE_SIZE || size 512 || !is_power_of_2(size)) return -EINVAL; @@ -126,18 +134,21 @@ int set_blocksize(struct block_device *b if (size bdev_logical_block_size(bdev)) return -EINVAL; + /* +* If the block size doesn't change, don't take the write lock. +* We check for is_bdev_mapped anyway, for consistent behavior. +*/ + if (size == bdev-bd_block_size) + return is_bdev_mapped(bdev) ? -EBUSY : 0; + /* Prevent starting I/O or mapping the device */ percpu_down_write(bdev-bd_block_size_semaphore); /* Check that the block device is not memory mapped */ - mapping = bdev-bd_inode-i_mapping; - mutex_lock(mapping-i_mmap_mutex); - if (mapping_mapped(mapping)) { - mutex_unlock(mapping-i_mmap_mutex); + if (is_bdev_mapped(bdev)) { percpu_up_write(bdev-bd_block_size_semaphore); return -EBUSY; } - mutex_unlock(mapping-i_mmap_mutex); /* Don't change the size if it is same as current */ if (bdev-bd_block_size != size) { This patch didn't really make any difference for ext2/3/4 but for reiserfs it does. With the synchronize_sched_expedited() patch applied, it didn't make any difference. Thanks, Jeff -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] block_dev: don't take the write lock if block size doesn't change
On Wed, 28 Nov 2012, Jeff Chua wrote: On Wed, Nov 28, 2012 at 12:01 PM, Mikulas Patocka mpato...@redhat.com wrote: block_dev: don't take the write lock if block size doesn't change Taking the write lock has a big performance impact on the whole system (because of synchronize_sched_expedited). This patch avoids taking the write lock if the block size doesn't change (i.e. when mounting filesystem with block size equal to the default block size). The logic to test if the block device is mapped was moved to a separate function is_bdev_mapped to avoid code duplication. Signed-off-by: Mikulas Patocka mpato...@redhat.com --- fs/block_dev.c | 25 ++--- 1 file changed, 18 insertions(+), 7 deletions(-) Index: linux-3.7-rc7/fs/block_dev.c === --- linux-3.7-rc7.orig/fs/block_dev.c 2012-11-28 04:09:01.0 +0100 +++ linux-3.7-rc7/fs/block_dev.c2012-11-28 04:13:53.0 +0100 @@ -114,10 +114,18 @@ void invalidate_bdev(struct block_device } EXPORT_SYMBOL(invalidate_bdev); -int set_blocksize(struct block_device *bdev, int size) +static int is_bdev_mapped(struct block_device *bdev) { - struct address_space *mapping; + int ret_val; + struct address_space *mapping = bdev-bd_inode-i_mapping; + mutex_lock(mapping-i_mmap_mutex); + ret_val = mapping_mapped(mapping); + mutex_unlock(mapping-i_mmap_mutex); + return ret_val; +} +int set_blocksize(struct block_device *bdev, int size) +{ /* Size must be a power of two, and between 512 and PAGE_SIZE */ if (size PAGE_SIZE || size 512 || !is_power_of_2(size)) return -EINVAL; @@ -126,18 +134,21 @@ int set_blocksize(struct block_device *b if (size bdev_logical_block_size(bdev)) return -EINVAL; + /* +* If the block size doesn't change, don't take the write lock. +* We check for is_bdev_mapped anyway, for consistent behavior. +*/ + if (size == bdev-bd_block_size) + return is_bdev_mapped(bdev) ? -EBUSY : 0; + /* Prevent starting I/O or mapping the device */ percpu_down_write(bdev-bd_block_size_semaphore); /* Check that the block device is not memory mapped */ - mapping = bdev-bd_inode-i_mapping; - mutex_lock(mapping-i_mmap_mutex); - if (mapping_mapped(mapping)) { - mutex_unlock(mapping-i_mmap_mutex); + if (is_bdev_mapped(bdev)) { percpu_up_write(bdev-bd_block_size_semaphore); return -EBUSY; } - mutex_unlock(mapping-i_mmap_mutex); /* Don't change the size if it is same as current */ if (bdev-bd_block_size != size) { This patch didn't really make any difference for ext2/3/4 but for reiserfs it does. With the synchronize_sched_expedited() patch applied, it didn't make any difference. Thanks, Jeff It could make difference for computers with many cores - synchronize_sched_expedited() is expensive there because it synchronizes all active cores, so if we can avoid it, just do it. Mikulas -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/2] block_dev: don't take the write lock if block size doesn't change
block_dev: don't take the write lock if block size doesn't change Taking the write lock has a big performance impact on the whole system (because of synchronize_sched_expedited). This patch avoids taking the write lock if the block size doesn't change (i.e. when mounting filesystem with block size equal to the default block size). The logic to test if the block device is mapped was moved to a separate function is_bdev_mapped to avoid code duplication. Signed-off-by: Mikulas Patocka --- fs/block_dev.c | 25 ++--- 1 file changed, 18 insertions(+), 7 deletions(-) Index: linux-3.7-rc7/fs/block_dev.c === --- linux-3.7-rc7.orig/fs/block_dev.c 2012-11-28 04:09:01.0 +0100 +++ linux-3.7-rc7/fs/block_dev.c2012-11-28 04:13:53.0 +0100 @@ -114,10 +114,18 @@ void invalidate_bdev(struct block_device } EXPORT_SYMBOL(invalidate_bdev); -int set_blocksize(struct block_device *bdev, int size) +static int is_bdev_mapped(struct block_device *bdev) { - struct address_space *mapping; + int ret_val; + struct address_space *mapping = bdev->bd_inode->i_mapping; + mutex_lock(>i_mmap_mutex); + ret_val = mapping_mapped(mapping); + mutex_unlock(>i_mmap_mutex); + return ret_val; +} +int set_blocksize(struct block_device *bdev, int size) +{ /* Size must be a power of two, and between 512 and PAGE_SIZE */ if (size > PAGE_SIZE || size < 512 || !is_power_of_2(size)) return -EINVAL; @@ -126,18 +134,21 @@ int set_blocksize(struct block_device *b if (size < bdev_logical_block_size(bdev)) return -EINVAL; + /* +* If the block size doesn't change, don't take the write lock. +* We check for is_bdev_mapped anyway, for consistent behavior. +*/ + if (size == bdev->bd_block_size) + return is_bdev_mapped(bdev) ? -EBUSY : 0; + /* Prevent starting I/O or mapping the device */ percpu_down_write(>bd_block_size_semaphore); /* Check that the block device is not memory mapped */ - mapping = bdev->bd_inode->i_mapping; - mutex_lock(>i_mmap_mutex); - if (mapping_mapped(mapping)) { - mutex_unlock(>i_mmap_mutex); + if (is_bdev_mapped(bdev)) { percpu_up_write(>bd_block_size_semaphore); return -EBUSY; } - mutex_unlock(>i_mmap_mutex); /* Don't change the size if it is same as current */ if (bdev->bd_block_size != size) { -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/2] block_dev: don't take the write lock if block size doesn't change
block_dev: don't take the write lock if block size doesn't change Taking the write lock has a big performance impact on the whole system (because of synchronize_sched_expedited). This patch avoids taking the write lock if the block size doesn't change (i.e. when mounting filesystem with block size equal to the default block size). The logic to test if the block device is mapped was moved to a separate function is_bdev_mapped to avoid code duplication. Signed-off-by: Mikulas Patocka mpato...@redhat.com --- fs/block_dev.c | 25 ++--- 1 file changed, 18 insertions(+), 7 deletions(-) Index: linux-3.7-rc7/fs/block_dev.c === --- linux-3.7-rc7.orig/fs/block_dev.c 2012-11-28 04:09:01.0 +0100 +++ linux-3.7-rc7/fs/block_dev.c2012-11-28 04:13:53.0 +0100 @@ -114,10 +114,18 @@ void invalidate_bdev(struct block_device } EXPORT_SYMBOL(invalidate_bdev); -int set_blocksize(struct block_device *bdev, int size) +static int is_bdev_mapped(struct block_device *bdev) { - struct address_space *mapping; + int ret_val; + struct address_space *mapping = bdev-bd_inode-i_mapping; + mutex_lock(mapping-i_mmap_mutex); + ret_val = mapping_mapped(mapping); + mutex_unlock(mapping-i_mmap_mutex); + return ret_val; +} +int set_blocksize(struct block_device *bdev, int size) +{ /* Size must be a power of two, and between 512 and PAGE_SIZE */ if (size PAGE_SIZE || size 512 || !is_power_of_2(size)) return -EINVAL; @@ -126,18 +134,21 @@ int set_blocksize(struct block_device *b if (size bdev_logical_block_size(bdev)) return -EINVAL; + /* +* If the block size doesn't change, don't take the write lock. +* We check for is_bdev_mapped anyway, for consistent behavior. +*/ + if (size == bdev-bd_block_size) + return is_bdev_mapped(bdev) ? -EBUSY : 0; + /* Prevent starting I/O or mapping the device */ percpu_down_write(bdev-bd_block_size_semaphore); /* Check that the block device is not memory mapped */ - mapping = bdev-bd_inode-i_mapping; - mutex_lock(mapping-i_mmap_mutex); - if (mapping_mapped(mapping)) { - mutex_unlock(mapping-i_mmap_mutex); + if (is_bdev_mapped(bdev)) { percpu_up_write(bdev-bd_block_size_semaphore); return -EBUSY; } - mutex_unlock(mapping-i_mmap_mutex); /* Don't change the size if it is same as current */ if (bdev-bd_block_size != size) { -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/