Re: [PATCH 9/9] tcm_qla2xxx: Add special case for COMPARE_AND_WRITE data_direction
On Wed, 2013-08-21 at 08:53 -0700, Christoph Hellwig wrote: > On Wed, Aug 21, 2013 at 07:38:21AM -0700, Roland Dreier wrote: > > I don't understand this. In fact the whole patch series looks quite > > confused. COMPARE AND WRITE is a normal Data-Out command, with no > > requirement for special bidirectional handling or anything like that. > > The only slightly unusual thing is that a CAW command with a NUMBER OF > > LOGICAL BLOCKS equal to N will actually transfer 2*N worth of data -- > > one set of data for the compare operation and a second set to write if > > the compare succeeds. But just to be clear, the transfer of those 2*N > > blocks happens as a single transfer during the Data-Out phase. > > I think the confusion is that the implementation of COMPARE AND WRITE > obviously requires a read and a write phase, and the implementation > tries to mix this up with an actual bidirectional scsi command. > > If the core stopped keying off t_bidi_data_sg and used better flag > this could be easily solved. Good point here as well.. Changing these cases to check for SCF_BIDI instead, and adding a extra SCF_COMPARE_AND_WRITE check for the case in transport_generic_new_cmd() to call transport_generic_get_mem_bidi(). --nab -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 9/9] tcm_qla2xxx: Add special case for COMPARE_AND_WRITE data_direction
On Wed, Aug 21, 2013 at 7:38 AM, Roland Dreier wrote: > I don't understand this. In fact the whole patch series looks quite > confused. COMPARE AND WRITE is a normal Data-Out command, with no > requirement for special bidirectional handling or anything like that. > The only slightly unusual thing is that a CAW command with a NUMBER OF > LOGICAL BLOCKS equal to N will actually transfer 2*N worth of data -- > one set of data for the compare operation and a second set to write if > the compare succeeds. But just to be clear, the transfer of those 2*N > blocks happens as a single transfer during the Data-Out phase. OK, I understand the patch set a bit better. You're using the bidi infrastructure to have a place to stick the data that you internally read to implement the compare, but then you end up having places like this where you have to say, "oh it's not really a bidi command, it's just a compare and write." Shouldn't there be a way to confine the COMPARE AND WRITE handling to the actual implementation of that command? Or maybe make the bidi handling more generic so that this becomes clearer? - R. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 9/9] tcm_qla2xxx: Add special case for COMPARE_AND_WRITE data_direction
On Wed, Aug 21, 2013 at 12:31:07AM -0700, Nicholas A. Bellinger wrote: > Is it really worth having two se_cmd_flags for COMPARE_AND_WRITE..? Not leaking the abstraction into the driver is always worth the effort. But looking at the other patches I haven't reviewed yet I think the issue is more severe anyway, see my next reply. > > Also it might make sense to lift this helper to get a dma direction from > > a command into common code. > > > > Mmm, perhaps. I don't recall of the top of my head why tcm_qla2xxx > actually needed to reverse it's dma direction (I'm sure that Roland > knows, CC'ed), but IIRC it was a tcm_qla2xxx specific thing..? It's the same issue for any hardware driver that directly maps a se_cmd - the direction the target expects is reversed to what the driver expects, in addition any BIDI or other special meanings will need handling. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 9/9] tcm_qla2xxx: Add special case for COMPARE_AND_WRITE data_direction
On Wed, Aug 21, 2013 at 07:38:21AM -0700, Roland Dreier wrote: > I don't understand this. In fact the whole patch series looks quite > confused. COMPARE AND WRITE is a normal Data-Out command, with no > requirement for special bidirectional handling or anything like that. > The only slightly unusual thing is that a CAW command with a NUMBER OF > LOGICAL BLOCKS equal to N will actually transfer 2*N worth of data -- > one set of data for the compare operation and a second set to write if > the compare succeeds. But just to be clear, the transfer of those 2*N > blocks happens as a single transfer during the Data-Out phase. I think the confusion is that the implementation of COMPARE AND WRITE obviously requires a read and a write phase, and the implementation tries to mix this up with an actual bidirectional scsi command. If the core stopped keying off t_bidi_data_sg and used better flag this could be easily solved. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 9/9] tcm_qla2xxx: Add special case for COMPARE_AND_WRITE data_direction
On Tue, Aug 20, 2013 at 1:08 PM, Nicholas A. Bellinger wrote: > Add a special case for COMPARE_AND_WRITE for the reverse data direction > mapping used for pci_map_sg() + friends. I don't understand this. In fact the whole patch series looks quite confused. COMPARE AND WRITE is a normal Data-Out command, with no requirement for special bidirectional handling or anything like that. The only slightly unusual thing is that a CAW command with a NUMBER OF LOGICAL BLOCKS equal to N will actually transfer 2*N worth of data -- one set of data for the compare operation and a second set to write if the compare succeeds. But just to be clear, the transfer of those 2*N blocks happens as a single transfer during the Data-Out phase. - R. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 9/9] tcm_qla2xxx: Add special case for COMPARE_AND_WRITE data_direction
On Tue, 2013-08-20 at 23:37 -0700, Christoph Hellwig wrote: > On Tue, Aug 20, 2013 at 08:08:00PM +, Nicholas A. Bellinger wrote: > > From: Nicholas Bellinger > > > > Add a special case for COMPARE_AND_WRITE for the reverse data direction > > mapping used for pci_map_sg() + friends. > > A low level driver is an even worse place to hardcode a specific cdb > opcode. As written before this should be done by a flag on the command. > Which means adding another COMPARE_AND_WRITE specific flag to se_cmd_flags_Table, as the SCF_COMPARE_AND_WRITE_POST is ony set after the comparsion of the verify instance data is complete.. Is it really worth having two se_cmd_flags for COMPARE_AND_WRITE..? > Also it might make sense to lift this helper to get a dma direction from > a command into common code. > Mmm, perhaps. I don't recall of the top of my head why tcm_qla2xxx actually needed to reverse it's dma direction (I'm sure that Roland knows, CC'ed), but IIRC it was a tcm_qla2xxx specific thing..? --nab -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 9/9] tcm_qla2xxx: Add special case for COMPARE_AND_WRITE data_direction
On Tue, Aug 20, 2013 at 08:08:00PM +, Nicholas A. Bellinger wrote: > From: Nicholas Bellinger > > Add a special case for COMPARE_AND_WRITE for the reverse data direction > mapping used for pci_map_sg() + friends. A low level driver is an even worse place to hardcode a specific cdb opcode. As written before this should be done by a flag on the command. Also it might make sense to lift this helper to get a dma direction from a command into common code. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 9/9] tcm_qla2xxx: Add special case for COMPARE_AND_WRITE data_direction
On Tue, Aug 20, 2013 at 08:08:00PM +, Nicholas A. Bellinger wrote: From: Nicholas Bellinger n...@daterainc.com Add a special case for COMPARE_AND_WRITE for the reverse data direction mapping used for pci_map_sg() + friends. A low level driver is an even worse place to hardcode a specific cdb opcode. As written before this should be done by a flag on the command. Also it might make sense to lift this helper to get a dma direction from a command into common code. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 9/9] tcm_qla2xxx: Add special case for COMPARE_AND_WRITE data_direction
On Tue, 2013-08-20 at 23:37 -0700, Christoph Hellwig wrote: On Tue, Aug 20, 2013 at 08:08:00PM +, Nicholas A. Bellinger wrote: From: Nicholas Bellinger n...@daterainc.com Add a special case for COMPARE_AND_WRITE for the reverse data direction mapping used for pci_map_sg() + friends. A low level driver is an even worse place to hardcode a specific cdb opcode. As written before this should be done by a flag on the command. Which means adding another COMPARE_AND_WRITE specific flag to se_cmd_flags_Table, as the SCF_COMPARE_AND_WRITE_POST is ony set after the comparsion of the verify instance data is complete.. Is it really worth having two se_cmd_flags for COMPARE_AND_WRITE..? Also it might make sense to lift this helper to get a dma direction from a command into common code. Mmm, perhaps. I don't recall of the top of my head why tcm_qla2xxx actually needed to reverse it's dma direction (I'm sure that Roland knows, CC'ed), but IIRC it was a tcm_qla2xxx specific thing..? --nab -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 9/9] tcm_qla2xxx: Add special case for COMPARE_AND_WRITE data_direction
On Tue, Aug 20, 2013 at 1:08 PM, Nicholas A. Bellinger n...@daterainc.com wrote: Add a special case for COMPARE_AND_WRITE for the reverse data direction mapping used for pci_map_sg() + friends. I don't understand this. In fact the whole patch series looks quite confused. COMPARE AND WRITE is a normal Data-Out command, with no requirement for special bidirectional handling or anything like that. The only slightly unusual thing is that a CAW command with a NUMBER OF LOGICAL BLOCKS equal to N will actually transfer 2*N worth of data -- one set of data for the compare operation and a second set to write if the compare succeeds. But just to be clear, the transfer of those 2*N blocks happens as a single transfer during the Data-Out phase. - R. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 9/9] tcm_qla2xxx: Add special case for COMPARE_AND_WRITE data_direction
On Wed, Aug 21, 2013 at 07:38:21AM -0700, Roland Dreier wrote: I don't understand this. In fact the whole patch series looks quite confused. COMPARE AND WRITE is a normal Data-Out command, with no requirement for special bidirectional handling or anything like that. The only slightly unusual thing is that a CAW command with a NUMBER OF LOGICAL BLOCKS equal to N will actually transfer 2*N worth of data -- one set of data for the compare operation and a second set to write if the compare succeeds. But just to be clear, the transfer of those 2*N blocks happens as a single transfer during the Data-Out phase. I think the confusion is that the implementation of COMPARE AND WRITE obviously requires a read and a write phase, and the implementation tries to mix this up with an actual bidirectional scsi command. If the core stopped keying off t_bidi_data_sg and used better flag this could be easily solved. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 9/9] tcm_qla2xxx: Add special case for COMPARE_AND_WRITE data_direction
On Wed, Aug 21, 2013 at 12:31:07AM -0700, Nicholas A. Bellinger wrote: Is it really worth having two se_cmd_flags for COMPARE_AND_WRITE..? Not leaking the abstraction into the driver is always worth the effort. But looking at the other patches I haven't reviewed yet I think the issue is more severe anyway, see my next reply. Also it might make sense to lift this helper to get a dma direction from a command into common code. Mmm, perhaps. I don't recall of the top of my head why tcm_qla2xxx actually needed to reverse it's dma direction (I'm sure that Roland knows, CC'ed), but IIRC it was a tcm_qla2xxx specific thing..? It's the same issue for any hardware driver that directly maps a se_cmd - the direction the target expects is reversed to what the driver expects, in addition any BIDI or other special meanings will need handling. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 9/9] tcm_qla2xxx: Add special case for COMPARE_AND_WRITE data_direction
On Wed, Aug 21, 2013 at 7:38 AM, Roland Dreier rol...@purestorage.com wrote: I don't understand this. In fact the whole patch series looks quite confused. COMPARE AND WRITE is a normal Data-Out command, with no requirement for special bidirectional handling or anything like that. The only slightly unusual thing is that a CAW command with a NUMBER OF LOGICAL BLOCKS equal to N will actually transfer 2*N worth of data -- one set of data for the compare operation and a second set to write if the compare succeeds. But just to be clear, the transfer of those 2*N blocks happens as a single transfer during the Data-Out phase. OK, I understand the patch set a bit better. You're using the bidi infrastructure to have a place to stick the data that you internally read to implement the compare, but then you end up having places like this where you have to say, oh it's not really a bidi command, it's just a compare and write. Shouldn't there be a way to confine the COMPARE AND WRITE handling to the actual implementation of that command? Or maybe make the bidi handling more generic so that this becomes clearer? - R. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 9/9] tcm_qla2xxx: Add special case for COMPARE_AND_WRITE data_direction
On Wed, 2013-08-21 at 08:53 -0700, Christoph Hellwig wrote: On Wed, Aug 21, 2013 at 07:38:21AM -0700, Roland Dreier wrote: I don't understand this. In fact the whole patch series looks quite confused. COMPARE AND WRITE is a normal Data-Out command, with no requirement for special bidirectional handling or anything like that. The only slightly unusual thing is that a CAW command with a NUMBER OF LOGICAL BLOCKS equal to N will actually transfer 2*N worth of data -- one set of data for the compare operation and a second set to write if the compare succeeds. But just to be clear, the transfer of those 2*N blocks happens as a single transfer during the Data-Out phase. I think the confusion is that the implementation of COMPARE AND WRITE obviously requires a read and a write phase, and the implementation tries to mix this up with an actual bidirectional scsi command. If the core stopped keying off t_bidi_data_sg and used better flag this could be easily solved. Good point here as well.. Changing these cases to check for SCF_BIDI instead, and adding a extra SCF_COMPARE_AND_WRITE check for the case in transport_generic_new_cmd() to call transport_generic_get_mem_bidi(). --nab -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
