Linux compat 4.18: check_disk_size_change() #7611
Conversation
behlendorf
force-pushed
the
check_events
branch
2 times, most recently
from
929e87f
to
72f512c
Compare
|
@bprotopopov would you mind reviewing this change, and if possible stress testing it for possible new locking issues introduced by the |
|
This seems reasonable to me after an initial pass, though I'll keep looking. Have you been able to verify that |
|
@shartse thanks for looking at this. I manually verified that the "change" events were being generated based on the console |
|
Yes, I’ll take a look, @behlendorf |
| ASSERT(!MUTEX_HELD(&zvol_state_lock)); | ||
|
|
||
| mutex_enter(&zvol_state_lock); | ||
| rw_enter(&zvol_state_lock, RW_READER); |
There was a problem hiding this comment.
Can we assert that zvol_state_lock is not held here ?
There was a problem hiding this comment.
Since this was converted to a krwlock it's now possible that another thread is holding the lock as a reader. The best we can do is assert that we're not holding it a writer but that wasn't really useful so I dropped it entirely.
There was a problem hiding this comment.
Right, it makes sense, thanks.
| ASSERT(!MUTEX_HELD(&zvol_state_lock)); | ||
|
|
||
| mutex_enter(&zvol_state_lock); | ||
| rw_enter(&zvol_state_lock, RW_READER); |
There was a problem hiding this comment.
Can we assert that zvol_state_lock is not held here ?
module/zfs/zvol.c
Outdated
|
|
||
| zvol_state_t *zv = disk->private_data; | ||
| if (zv != NULL) { | ||
| boolean_t locked = MUTEX_HELD(&zv->zv_state_lock); |
There was a problem hiding this comment.
For this function, as well as zvol_media_changed() and zvol_revalidate_disk(), I am not sure why we don't know if we are holding the zv_state_lock(). Can some comments be added here as to the possible locking states of the calling thread and the context for those states to take place?
There was a problem hiding this comment.
Good idea, will do.
Added support for the bops->check_events() interface which was added in the 2.6.38 kernel to replace bops->media_changed(). Fully implementing this functionality allows the volume resize code to rely on revalidate_disk(), which is the preferred mechanism, and removes the need to use check_disk_size_change(). In order for bops->check_events() to lookup the zvol_state_t stored in the disk->private_data the zvol_state_lock needs to be held. Since the check events interface may poll the mutex has been converted to a rwlock for better concurrently. The rwlock need only be taken as a writer in the zvol_free() path when disk->private_data is set to NULL. The configure checks for the block_device_operations structure were consolidated in a single kernel-block-device-operations.m4 file. The ZFS_AC_KERNEL_BDEV_BLOCK_DEVICE_OPERATIONS configure checks and assoicated dead code was removed. This interface was added to the 2.6.28 kernel which predates the oldest supported 2.6.32 kernel and will therefore always be available. Updated maximum Linux version in META file. The 4.17 kernel was released on 2018-06-03 and ZoL is compatible with the finalized kernel. Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
|
@bprotopopov @shartse refreshed. I've removed the |
|
|
||
| set_capacity(zv->zv_disk, volsize >> 9); | ||
| zv->zv_volsize = volsize; | ||
| check_disk_size_change(zv->zv_disk, bdev); |
There was a problem hiding this comment.
Is there anything we're losing from no longer using check_disk_size_change?
It looks like there's kernel logging:
printk(KERN_INFO, "%s: detected capacity change from %lld to %lld\n", disk->disk_name, bdev_size, disk_size);
and updates to the inode:
i_size_write(bdev->bd_inode, disk_size);
I'm not sure if the inode is meaningful to us in this case, I don't fully understand how it relates to azvol. I also don't know if kernel logging makes sense as a level of abstraction for a zvol but maybe we can get the size change logged in zfs_dbgmsg?
There was a problem hiding this comment.
@shartse good question. The short answer is we shouldn't be losing anything since revalidate_disk() will internally call check_disk_size_change(). In fact, revalidate_disk() is the interface we should have been using all along, and check_disk_size_change() was un-exported because it wasn't originally intended to be called by drivers.
As for logging the size change that's an interesting thought, what would be the use case for this?
In practice my feeling is it wouldn't be too useful, and we would already have some log information from the zpool history log entry.
There was a problem hiding this comment.
Cool - thanks for the clarification. I think having the information in zpool history is sufficient
Codecov Report
@@ Coverage Diff @@
## master #7611 +/- ##
==========================================
- Coverage 77.45% 77.44% -0.01%
==========================================
Files 363 363
Lines 110514 110517 +3
==========================================
- Hits 85595 85592 -3
- Misses 24919 24925 +6
Continue to review full report at Codecov.
|
Added support for the bops->check_events() interface which was added in the 2.6.38 kernel to replace bops->media_changed(). Fully implementing this functionality allows the volume resize code to rely on revalidate_disk(), which is the preferred mechanism, and removes the need to use check_disk_size_change(). In order for bops->check_events() to lookup the zvol_state_t stored in the disk->private_data the zvol_state_lock needs to be held. Since the check events interface may poll the mutex has been converted to a rwlock for better concurrently. The rwlock need only be taken as a writer in the zvol_free() path when disk->private_data is set to NULL. The configure checks for the block_device_operations structure were consolidated in a single kernel-block-device-operations.m4 file. The ZFS_AC_KERNEL_BDEV_BLOCK_DEVICE_OPERATIONS configure checks and assoicated dead code was removed. This interface was added to the 2.6.28 kernel which predates the oldest supported 2.6.32 kernel and will therefore always be available. Updated maximum Linux version in META file. The 4.17 kernel was released on 2018-06-03 and ZoL is compatible with the finalized kernel. Reviewed-by: Boris Protopopov <boris.protopopov@actifio.com> Reviewed-by: Sara Hartse <sara.hartse@delphix.com> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes openzfs#7611 Backported-by: Richard Yao <ryao@gentoo.org>
Added support for the bops->check_events() interface which was added in the 2.6.38 kernel to replace bops->media_changed(). Fully implementing this functionality allows the volume resize code to rely on revalidate_disk(), which is the preferred mechanism, and removes the need to use check_disk_size_change(). In order for bops->check_events() to lookup the zvol_state_t stored in the disk->private_data the zvol_state_lock needs to be held. Since the check events interface may poll the mutex has been converted to a rwlock for better concurrently. The rwlock need only be taken as a writer in the zvol_free() path when disk->private_data is set to NULL. The configure checks for the block_device_operations structure were consolidated in a single kernel-block-device-operations.m4 file. The ZFS_AC_KERNEL_BDEV_BLOCK_DEVICE_OPERATIONS configure checks and assoicated dead code was removed. This interface was added to the 2.6.28 kernel which predates the oldest supported 2.6.32 kernel and will therefore always be available. Updated maximum Linux version in META file. The 4.17 kernel was released on 2018-06-03 and ZoL is compatible with the finalized kernel. Reviewed-by: Boris Protopopov <boris.protopopov@actifio.com> Reviewed-by: Sara Hartse <sara.hartse@delphix.com> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes openzfs#7611 Backported-by: Richard Yao <ryao@gentoo.org>
Added support for the bops->check_events() interface which was added in the 2.6.38 kernel to replace bops->media_changed(). Fully implementing this functionality allows the volume resize code to rely on revalidate_disk(), which is the preferred mechanism, and removes the need to use check_disk_size_change(). In order for bops->check_events() to lookup the zvol_state_t stored in the disk->private_data the zvol_state_lock needs to be held. Since the check events interface may poll the mutex has been converted to a rwlock for better concurrently. The rwlock need only be taken as a writer in the zvol_free() path when disk->private_data is set to NULL. The configure checks for the block_device_operations structure were consolidated in a single kernel-block-device-operations.m4 file. The ZFS_AC_KERNEL_BDEV_BLOCK_DEVICE_OPERATIONS configure checks and assoicated dead code was removed. This interface was added to the 2.6.28 kernel which predates the oldest supported 2.6.32 kernel and will therefore always be available. Updated maximum Linux version in META file. The 4.17 kernel was released on 2018-06-03 and ZoL is compatible with the finalized kernel. Reviewed-by: Boris Protopopov <boris.protopopov@actifio.com> Reviewed-by: Sara Hartse <sara.hartse@delphix.com> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes openzfs#7611
Added support for the bops->check_events() interface which was added in the 2.6.38 kernel to replace bops->media_changed(). Fully implementing this functionality allows the volume resize code to rely on revalidate_disk(), which is the preferred mechanism, and removes the need to use check_disk_size_change(). In order for bops->check_events() to lookup the zvol_state_t stored in the disk->private_data the zvol_state_lock needs to be held. Since the check events interface may poll the mutex has been converted to a rwlock for better concurrently. The rwlock need only be taken as a writer in the zvol_free() path when disk->private_data is set to NULL. The configure checks for the block_device_operations structure were consolidated in a single kernel-block-device-operations.m4 file. The ZFS_AC_KERNEL_BDEV_BLOCK_DEVICE_OPERATIONS configure checks and assoicated dead code was removed. This interface was added to the 2.6.28 kernel which predates the oldest supported 2.6.32 kernel and will therefore always be available. Updated maximum Linux version in META file. The 4.17 kernel was released on 2018-06-03 and ZoL is compatible with the finalized kernel. Reviewed-by: Boris Protopopov <boris.protopopov@actifio.com> Reviewed-by: Sara Hartse <sara.hartse@delphix.com> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes openzfs#7611
Description
Added support for the bops->check_events() interface which was added in the 2.6.38 kernel to replace bops->media_changed(). Fully implementing this functionality allows the volume resize code to rely on revalidate_disk(), which is the preferred mechanism, and removes the need to use check_disk_size_change().
In order for bops->check_events() to lookup the zvol_state_t stored in the disk->private_data the zvol_state_lock needs to be held. Since the check events interface may poll the mutex has been converted to a rwlock for better concurrency. The rwlock need only be taken as a writer in the zvol_free() path when disk->private_data is set to NULL.
The configure checks for the block_device_operations structure were consolidated in a single kernel-block-device-operations.m4 file.
The ZFS_AC_KERNEL_BDEV_BLOCK_DEVICE_OPERATIONS configure checks and assoicated dead code was removed. This interface was added to the 2.6.28 kernel which predates the oldest supported 2.6.32 kernel and will therefore always be available.
Updated maximum Linux version in META file. The 4.17 kernel was released on 2018-06-03 and ZoL is compatible with the finalized kernel.
Motivation and Context
Maintain compatibility with the latest kernels.
How Has This Been Tested?
Locally built and manually tested that change events are being generated for resized volumes. Relying on the bots to verify functionality for populate kernel versions. Locally, all basic
zvoltests pass.Types of changes
Checklist:
Signed-off-by.