Re: [PATCHv2 0/12] target_core_pr.c cleanups

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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?

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.

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 X years later we remove it.

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.

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.

-- 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




[Index of Archives]     [Linux SCSI]     [Kernel Newbies]     [Linux SCSI Target Infrastructure]     [Share Photos]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Device Mapper]

  Powered by Linux