On Fri, 2013-05-17 at 10:34 -0700, Andy Grover wrote: > 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. > No, because it assumes that userspace can poll configfs at anytime for the correct state of in-flight PR aptpl metadata, which is an incorrect assumption if you look closely at the code. > > 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. > That's a good start. > > 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. > The problem is that I've gotten large cleanup patches like this in the past that obviously have had zero testing at all. One example of this is commit f80e8ed3, which if you'd bother to do even basic testing on it would have been noticed immediately. Really, If you insist on churning working code for flexibilities sake, don't be surprised if I push back on the testing part. > 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. > The question is why is this needed in this case..? Why is it important enough to start adding multiple code-paths for saving point in time state of PR + ALUA metadata by adding userspace code dependencies within an SCSI I/O path..? If it's just matter of where the save state is located, there's lots of easier ways than adding a userspace up-call to an existing SCSI PR path. Any why on earth does user-space care about the flexibility of the formatting for this case..? Are the userspace consumers of PR meta-data really diverse enough that it needs stuff like this added for flexibility sake..? > 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. > Adding multiple ways of doing this in the kernel at the same time is just wrong. There's needs to be one consistent way, and that's what userspace needs to follow. --nab -- 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
