On Fri, 2013-05-17 at 15:36 -0700, Andy Grover wrote: > On 05/17/2013 01:12 PM, Nicholas A. Bellinger wrote: > > On Fri, 2013-05-17 at 10:34 -0700, Andy Grover wrote: > >>> 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. > > Well I see that it becomes very difficult to assure that aptpl metadata > changes will be saved if the system crashes less than a second after the > update. The current pr save method is subject to this as well, and seems > pretty unavoidable without killing performance. Is this the issue you're > referring to? No, it's the pulling out of aptpl metadata from configfs at random times that can lead to incorrect state. Just having that bit exposed in patch #12 doesn't make any sense given the NAK on the original #13, which is why I'm not applying it either. > > > 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. > > Working code is not good enough for the Linux kernel. It needs to be > maintainable, readable working code. > > I'm fine with you pushing back on the testing part. I'll include testing > summaries in my future patchsets. I'm not fine with you singling me out > for blame, because we ALL need to get better about this. > I'm singling you out because of things in the recent past that I had to fix due to general laziness, such as in commit f80e8ed3. Really, don't ever send large changes to key parts of I/O path code with bugs that would have be immediately spotted if you'd bother to run a single block of I/O before sending the changes my way. > > 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. > > It's not just that /var/targets/pr is a bad location, it's that the > kernel should not write to the fs *at all*. > > Do a 'git grep " kernel_write"'. We are on a short, short list of spots > that do this, and we need to find a way to stop, long term. I understand > existing user tools expect this, which is why my patch maintained > current behavior for now, then down the road we stick in a deprecation > printk, and then years later we remove it. > Huh..? kernel_write translates to vfs_write.. So your saying that these are going away long-term..? > > 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..? > > Policy should not go in the kernel. > Nonsense, this has nothing to do with policy. Whatever you want to do can be done with the existing code using inotify on the metadata files in question, with the exact same formatting as the changes you've already proposed. > > 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. > > We need a transition, and that means we have two for a while. Sorry, but I'll be NAKing anything that does an user-space upcall and keeps two diverging paths for user-space code. --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
