Skip to content
This repository has been archived by the owner on Feb 26, 2020. It is now read-only.

need a working rw_tryupgrade() #534

Closed
dweeezil opened this issue Mar 6, 2016 · 2 comments
Closed

need a working rw_tryupgrade() #534

dweeezil opened this issue Mar 6, 2016 · 2 comments

Comments

@dweeezil
Copy link
Contributor

dweeezil commented Mar 6, 2016

The lack of a working rw_tryupgrade() in all but the CONFIG_RWSEM_GENERIC_SPINLOCK case (which is pretty rare these days) can have ill effects if the upstream OpenZFS code uses it in certain performance-sensitive areas.

For example, openzfs/zfs@7f60329 introduced a statement which inhibits creation of new prefetch streams when rw_tryupgrade() returns zero.

The problem stems from the imperfect mapping between the Solaris krwlock facility and Linux's rw_semaphore facility (see d28db80 and others for details). In addition to the differing API, Linux does not natively support an upgrade from a reader lock to a reader/writer lock (but it does support the reverse).

Since the entire Solaris krwlock facility is implemented as macros which expand to statement expressions and also since the krwlock_t structure is a wrapper for rw_semaphore, there's a great deal of flexibility available within the SPL. It seems that, for example, it might be possible to add additional members to krwlock_t to help track such things as pending waiters for write or any other accounting and/or spinlocks which might be useful to implement a semantically equivalent rw_tryupgrade().

[EDIT] Referencing openzfs/zfs#4388.

@dweeezil
Copy link
Contributor Author

dweeezil commented Mar 7, 2016

After auditing the other users of rw_tryupgrade() in ZFS, it doesn't seem this is worth doing at the moment. The code mentioned in openzfs/zfs@7f60329 is the only place in which it's used without effectively immediately dropping and re-acquiring a RW lock in the failure case.

@dweeezil
Copy link
Contributor Author

dweeezil commented Mar 8, 2016

Closing. As indicated in openzfs/zfs#4388 (comment) and also from my own audit, this isn't worth implementing, but at least it's mentioned here for documentation in the future.

@dweeezil dweeezil closed this as completed Mar 8, 2016
behlendorf added a commit to behlendorf/spl that referenced this issue Mar 9, 2016
This implementation of rw_tryupgrade() behaves slightly differently
from its counterparts on other platforms.  It drops the RW_READER lock
and then acquires the RW_WRITER lock leaving a small window where no
lock is held.  On other platforms the lock is never released during
the upgrade process.  This is necessary under Linux because the kernel
does not provide an upgrade function.

There are currently no callers in the ZFS code where this change in
behavior is a problem.  In fact, in most cases the code is already
written such that if the upgrade fails the RW_READER lock is dropped
and the caller blocks waiting to acquire the lock as RW_WRITER.

Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Issue openzfs/zfs#4388
Issue openzfs#534
behlendorf added a commit that referenced this issue Mar 10, 2016
This implementation of rw_tryupgrade() behaves slightly differently
from its counterparts on other platforms.  It drops the RW_READER lock
and then acquires the RW_WRITER lock leaving a small window where no
lock is held.  On other platforms the lock is never released during
the upgrade process.  This is necessary under Linux because the kernel
does not provide an upgrade function.

There are currently no callers in the ZFS code where this change in
behavior is a problem.  In fact, in most cases the code is already
written such that if the upgrade fails the RW_READER lock is dropped
and the caller blocks waiting to acquire the lock as RW_WRITER.

Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Tim Chase <tim@chase2k.com>
Signed-off-by: Matthew Thode <prometheanfire@gentoo.org>
Closes openzfs/zfs#4388
Closes #534
nedbass pushed a commit to nedbass/spl that referenced this issue Aug 26, 2016
This implementation of rw_tryupgrade() behaves slightly differently
from its counterparts on other platforms.  It drops the RW_READER lock
and then acquires the RW_WRITER lock leaving a small window where no
lock is held.  On other platforms the lock is never released during
the upgrade process.  This is necessary under Linux because the kernel
does not provide an upgrade function.

There are currently no callers in the ZFS code where this change in
behavior is a problem.  In fact, in most cases the code is already
written such that if the upgrade fails the RW_READER lock is dropped
and the caller blocks waiting to acquire the lock as RW_WRITER.

Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Tim Chase <tim@chase2k.com>
Signed-off-by: Matthew Thode <prometheanfire@gentoo.org>
Closes openzfs/zfs#4388
Closes openzfs#534
tuxoko pushed a commit to tuxoko/spl that referenced this issue Sep 8, 2016
This implementation of rw_tryupgrade() behaves slightly differently
from its counterparts on other platforms.  It drops the RW_READER lock
and then acquires the RW_WRITER lock leaving a small window where no
lock is held.  On other platforms the lock is never released during
the upgrade process.  This is necessary under Linux because the kernel
does not provide an upgrade function.

There are currently no callers in the ZFS code where this change in
behavior is a problem.  In fact, in most cases the code is already
written such that if the upgrade fails the RW_READER lock is dropped
and the caller blocks waiting to acquire the lock as RW_WRITER.

Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Tim Chase <tim@chase2k.com>
Signed-off-by: Matthew Thode <prometheanfire@gentoo.org>
Closes openzfs/zfs#4388
Closes openzfs#534
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant