On 05/16/2013 05:18 PM, Nicholas A. Bellinger wrote:
On Thu, 2013-05-16 at 10:40 -0700, Andy Grover wrote:
Changes from v1:
* Drop #4 "Only return 0 or -err from core_scsi3_check_implicit_release"
* Split up #10 (9 and 10 in this series)
* #13 "Add upcall to save aptpl state" held back for separate discussion
So I'm mostly OK with these.
The one exception is with patch #12, which given the NAK on the upcall
junk doesn't make sense to apply as is.
Well PR and ALUA are the only two spots in LIO where config is not in
configfs. Shouldn't we start to fix this? This patch exposes PR, and it
only adds 3 lines of code, seems like a good start.
Can you please share how you've been testing these changes,
specifically the aptpl bits..?
I created a loopback device and poked it with sg_persist -Z, setting and
clearing PRs. The writes to /var/target/pr still worked when
/sbin/target_notify was absent, and were properly omitted when present
and the upcall returned success. The configfs change worked as well.
From now on I'm going to be alot more strict is taking changes like this
that are only compile tested, given the number of regressions in the
past that have come up from your changes.
Just for the record I do *not* have the most regressions, and I view
falsely singling me out for public criticism as spillover from dislike
for the upcall patch.
Can we have a purely technical discussion about the upcall patch now,
please? You said:
Please don't add user-space upcalls when they don't provide a real
benefit. It just makes things needlessly more complex, and adds
pointless user-space dependencies for existing code.
The real benefit is removing the policy of where and how kernel state
gets saved by userspace from the kernel, similar to how udev leaves it
up to userspace on exactly how to layout /dev. The upcall gives
userspace the flexibility to store PR info however it likes, instead of
mandating a fixed /var/target/pr location and format.
It is completely seamless for existing userspace code. As long as
/sbin/target_notify doesn't exist then the kernel's behavior doesn't
change. Userspace has to opt-in, and then is responsible for saving and
restoring PR state across reboots without relying upon the files in
/var/target/pr.
I'm certainly open to other implementations that meet the same
requirements, but since we don't want to have a daemon running all the
time watching a socket for configuration changes, call_usermodehelper()
seemed like the right way to go.
Thoughts?
-- Andy
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
