[PATCH] IB/iser: fix max_sectors calculation

[PATCH] IB/iser: fix max_sectors calculation

[Christoph Hellwig]

iSER currently has a couple places that set max_sectors in either the host
template or SCSI host, and all of them get it wrong.

This patch instead uses a single assignment that (hopefully) gets it right:
the max_sectors value must be derived from the number of segments in the
FR or FRM structure, but actually be one lower than the page size multiplied
by the number of sectors, as it has to handle the case of non-aligned I/O.

Without this I get trivivial to reproduce hangs when running xfstests
(on XFS) over iSER to Linux targets.

Signed-off-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
---
 drivers/infiniband/ulp/iser/iscsi_iser.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c b/drivers/infiniband/ulp/iser/iscsi_iser.c
index 80b6bed..64b3d11 100644
--- a/drivers/infiniband/ulp/iser/iscsi_iser.c
+++ b/drivers/infiniband/ulp/iser/iscsi_iser.c
@@ -612,6 +612,7 @@ iscsi_iser_session_create(struct iscsi_endpoint *ep,
 	struct Scsi_Host *shost;
 	struct iser_conn *iser_conn = NULL;
 	struct ib_conn *ib_conn;
+	u32 max_fr_sectors;
 	u16 max_cmds;
 
 	shost = iscsi_host_alloc(&iscsi_iser_sht, 0, 0);
@@ -632,7 +633,6 @@ iscsi_iser_session_create(struct iscsi_endpoint *ep,
 		iser_conn = ep->dd_data;
 		max_cmds = iser_conn->max_cmds;
 		shost->sg_tablesize = iser_conn->scsi_sg_tablesize;
-		shost->max_sectors = iser_conn->scsi_max_sectors;
 
 		mutex_lock(&iser_conn->state_mutex);
 		if (iser_conn->state != ISER_CONN_UP) {
@@ -657,8 +657,6 @@ iscsi_iser_session_create(struct iscsi_endpoint *ep,
 		 */
 		shost->sg_tablesize = min_t(unsigned short, shost->sg_tablesize,
 			ib_conn->device->ib_device->attrs.max_fast_reg_page_list_len);
-		shost->max_sectors = min_t(unsigned int,
-			1024, (shost->sg_tablesize * PAGE_SIZE) >> 9);
 
 		if (iscsi_host_add(shost,
 				   ib_conn->device->ib_device->dma_device)) {
@@ -672,6 +670,15 @@ iscsi_iser_session_create(struct iscsi_endpoint *ep,
 			goto free_host;
 	}
 
+	/*
+	 * FRs or FMRs can only map up to a (device) page per entry, but if the
+	 * first entry is misaligned we'll end up using using two entries
+	 * (head and tail) for a single page worth data, so we have to drop
+	 * one segment from the calculation.
+	 */
+	max_fr_sectors = ((shost->sg_tablesize - 1) * PAGE_SIZE) >> 9;
+	shost->max_sectors = min(iser_max_sectors, max_fr_sectors);
+
 	if (cmds_max > max_cmds) {
 		iser_info("cmds_max changed from %u to %u\n",
 			  cmds_max, max_cmds);
@@ -989,7 +996,6 @@ static struct scsi_host_template iscsi_iser_sht = {
 	.queuecommand           = iscsi_queuecommand,
 	.change_queue_depth	= scsi_change_queue_depth,
 	.sg_tablesize           = ISCSI_ISER_DEF_SG_TABLESIZE,
-	.max_sectors            = ISER_DEF_MAX_SECTORS,
 	.cmd_per_lun            = ISER_DEF_CMD_PER_LUN,
 	.eh_abort_handler       = iscsi_eh_abort,
 	.eh_device_reset_handler= iscsi_eh_device_reset,
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/iser: fix max_sectors calculation

[Sagi Grimberg]

Thanks Christoph,

Acked-by: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>

Doug, this needs a stable tag.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/iser: fix max_sectors calculation

[Max Gurtovoy]

On 4/19/2016 12:06 AM, Christoph Hellwig wrote:
> iSER currently has a couple places that set max_sectors in either the host
> template or SCSI host, and all of them get it wrong.
>
> This patch instead uses a single assignment that (hopefully) gets it right:
> the max_sectors value must be derived from the number of segments in the
> FR or FRM structure, but actually be one lower than the page size multiplied

* FRM ==> FMR

> by the number of sectors, as it has to handle the case of non-aligned I/O.
>
> Without this I get trivivial to reproduce hangs when running xfstests
> (on XFS) over iSER to Linux targets.
>
> Signed-off-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
> ---
>   drivers/infiniband/ulp/iser/iscsi_iser.c | 14 ++++++++++----
>   1 file changed, 10 insertions(+), 4 deletions(-)
>

Looks good.

Reviewed-by: Max Gurtovoy <maxg-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/iser: fix max_sectors calculation

[Or Gerlitz]

On Tue, Apr 19, 2016 at 12:06 AM, Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org> wrote:
> iSER currently has a couple places that set max_sectors in either the host
> template or SCSI host, and all of them get it wrong.
>
> This patch instead uses a single assignment that (hopefully) gets it right:
> the max_sectors value must be derived from the number of segments in the
> FR or FRM structure, but actually be one lower than the page size multiplied
> by the number of sectors, as it has to handle the case of non-aligned I/O.
>
> Without this I get trivivial to reproduce hangs when running xfstests
> (on XFS) over iSER to Linux targets.

nit, trivivial  --> trivial, please fix

Sagi, Christoph,

You mentioned stable kernels, can you  spot the breakage point?
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/iser: fix max_sectors calculation

[Or Gerlitz]

On Tue, Apr 19, 2016 at 12:06 AM, Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org> wrote:
> iSER currently has a couple places that set max_sectors in either the host
> template or SCSI host, and all of them get it wrong.
>
> This patch instead uses a single assignment that (hopefully) gets it right:
> the max_sectors value must be derived from the number of segments in the
> FR or FRM structure, but actually be one lower than the page size multiplied
> by the number of sectors, as it has to handle the case of non-aligned I/O.
>
> Without this I get trivivial to reproduce hangs when running xfstests
> (on XFS) over iSER to Linux targets.

nit but please fix

 IB/iser: fix max_sectors calculation

-->
 IB/iser: Fix calculation of maximal number of sectors posted to SCSI layer
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/iser: fix max_sectors calculation

[Christoph Hellwig]

I haven't used that explicit language, but:  go fuck yourself, seriously.


If you as a maintained want to update changelogs to fit your taste go
for it (I do it all the time), but sending half a dozen mails to fix
minor changelog formatting for a stable patch that fixes a regression
just makes me angry.

I thus withdraw this patch and officially don't care about the broken
state of iSER anymore.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/iser: fix max_sectors calculation

[Or Gerlitz]

On Thu, Apr 28, 2016 at 10:55 AM, Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org> wrote:
> I haven't used that explicit language, but:  go fuck yourself, seriously.

Christoph, I wrote you on both cases this was a nit comment.

I understand now that you are not willing to accept nits as feedback on
your fixes, and that you are really nervous on me, happens.

I suggest you seek (and hopefully find) other means to get relaxed,
I know you are top kernel contributor but this doesn't justify talking like
this to me nor to anyone else, nor refusing to fix nits if asked by reviewers.

> If you as a maintained want to update changelogs to fit your taste go
> for it (I do it all the time), but sending half a dozen mails to fix
> minor changelog formatting for a stable patch that fixes a regression
> just makes me angry.

If you prefer it this way, sure, I can do that.

> I thus withdraw this patch and officially don't care about the broken
> state of iSER anymore.

Slow down.

As I said, taking into account the volume of your work on the driver,
if you prefer nits to be fixed by others we can do that.

Or.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/iser: fix max_sectors calculation

[Jason Gunthorpe]

On Thu, Apr 28, 2016 at 01:51:21PM +0300, Or Gerlitz wrote:
> On Thu, Apr 28, 2016 at 10:55 AM, Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org> wrote:
> > I haven't used that explicit language, but:  go fuck yourself, seriously.
> 
> Christoph, I wrote you on both cases this was a nit comment.
> 
> I understand now that you are not willing to accept nits as feedback on
> your fixes, and that you are really nervous on me, happens.

I wondered when this would finally blow up..

I would also prefer the nit picking on commit language to stop. It is
not a welcoming environment for new contributors, and it is a pain to
those who are doing the bulk of the work.

Get Doug to fix stuff up to match your preferences or let it go.

It is *really* not OK to ask Sagi to not ack such commits when the
code is OK. Sagi is doing a fine job.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/iser: fix max_sectors calculation

[Or Gerlitz]

On Thu, Apr 28, 2016 at 7:28 PM, Jason Gunthorpe
<jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote:
> On Thu, Apr 28, 2016 at 01:51:21PM +0300, Or Gerlitz wrote:
>> On Thu, Apr 28, 2016 at 10:55 AM, Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org> wrote:
>> > I haven't used that explicit language, but:  go fuck yourself, seriously.
>>
>> Christoph, I wrote you on both cases this was a nit comment.
>>
>> I understand now that you are not willing to accept nits as feedback on
>> your fixes, and that you are really nervous on me, happens.
>
> I wondered when this would finally blow up..

> I would also prefer the nit picking on commit language to stop. It is
> not a welcoming environment for new contributors, and it is a pain to
> those who are doing the bulk of the work.

So you are a native English speaker that prefers to have commit
change-logs with spelling mistakes vs. see a nit reviewer comment
followed by a respin by the submitter? your taste, I don't think this
is the way to go, the change-log stays there for ever and there's no
reason on earth not to fix spelling if we can do that throughout the
review. And, yes, I do think that new comers and old suffers need to
run spelling on their change-logs before submitting.

> Get Doug to fix stuff up to match your preferences or let it go.

How about you get Doug to merge for-next code prior to ten days into
the merge window? we are 10K KM from having that.

> It is *really* not OK to ask Sagi to not ack such commits when the
> code is OK. Sagi is doing a fine job.

I disagree. We have some conventions for that driver commits, and we
have the right to keep them. If Christoph prefers that we will do the
final touch, sure, we can do that.

And hey, thanks for writing that without cursing [1] me.

Or.

[1] being a non native English speaker, I wasn't sure if that the
right word, I mean, not telling go fuck yourself or alike on every 2nd
sentence emitted to the world
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/iser: fix max_sectors calculation

[Jason Gunthorpe]

On Thu, Apr 28, 2016 at 11:21:45PM +0300, Or Gerlitz wrote:
> > I would also prefer the nit picking on commit language to stop. It is
> > not a welcoming environment for new contributors, and it is a pain to
> > those who are doing the bulk of the work.
> 
> So you are a native English speaker that prefers to have commit
> change-logs with spelling mistakes vs. see a nit reviewer comment
> followed by a respin by the submitter?

What I do care about is growing our community and drawing in more high
quality contributions, so seeing good work derided with a bunch of
ugly emails is far, far worse in my mind.

I can read past a few minor capitalization, spelling and grammar
errors in a commit, and will happily accept that to improve our
community.

As a native speaker, I can tell you that the tone of the last batch of
emails you sent was very poor. Too accusatory and demanding for the
level of nitpicking you were doing.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/iser: fix max_sectors calculation

[Sagi Grimberg]

OK let's calm down...

Christoph, no need to get angry, Or as a reviewer can suggest a
change-log fix, and you as a contributor can simply ignore it if
you don't want to fix it, no reason for the harsh language.
As you said, Or can fix it if he really cares about it.

Or, I personally don't care that much about format/capitalization all
that much, but I know you do, so it's fine if we conform to that, but
we can fix it ourselves and ease up on the nit-picking in this area.

Thank you both, for contributing and reviewing!

Cheers,
Switzerland.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/iser: fix max_sectors calculation

[Or Gerlitz]

On Thu, Apr 28, 2016 at 2:15 PM, Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org> wrote:
> OK let's calm down...

[...]

> Or, I personally don't care that much about format/capitalization all
> that much, but I know you do, so it's fine if we conform to that, but
> we can fix it ourselves and ease up on the nit-picking in this area.

Makes sense to me, want me to pick this up here or you gonna?
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] IB/iser: fix max_sectors calculation

[Sagi Grimberg]

>> Or, I personally don't care that much about format/capitalization all
>> that much, but I know you do, so it's fine if we conform to that, but
>> we can fix it ourselves and ease up on the nit-picking in this area.
>
> Makes sense to me, want me to pick this up here or you gonna?

I'll take it with Doug, no problem.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html