| Message ID | 154c7602b3cc59f8af44439249ea5e5eb75f92d3.1675876734.git.lduncan@suse.com |
|---|---|
| State | New |
| Headers | show |
| Series | Make iscsid-kernel communications namespace-aware | expand |
On 2/8/23 18:40, Lee Duncan wrote: > From: Lee Duncan <lduncan@suse.com> > > Right now the iscsi_endpoint is only linked to a connection once that > connection has been established. For net namespace filtering of the > sysfs objects, associate an endpoint with the host that it was > allocated for when it is created. > > Signed-off-by: Chris Leech <cleech@redhat.com> > Signed-off-by: Lee Duncan <lduncan@suse.com> > --- > drivers/infiniband/ulp/iser/iscsi_iser.c | 2 +- > drivers/scsi/be2iscsi/be_iscsi.c | 2 +- > drivers/scsi/bnx2i/bnx2i_iscsi.c | 2 +- > drivers/scsi/cxgbi/libcxgbi.c | 2 +- > drivers/scsi/qedi/qedi_iscsi.c | 2 +- > drivers/scsi/qla4xxx/ql4_os.c | 2 +- > drivers/scsi/scsi_transport_iscsi.c | 3 ++- > include/scsi/scsi_transport_iscsi.h | 6 +++++- > 8 files changed, 13 insertions(+), 8 deletions(-) > > diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c b/drivers/infiniband/ulp/iser/iscsi_iser.c > index 620ae5b2d80d..d38c909b462f 100644 > --- a/drivers/infiniband/ulp/iser/iscsi_iser.c > +++ b/drivers/infiniband/ulp/iser/iscsi_iser.c > @@ -802,7 +802,7 @@ static struct iscsi_endpoint *iscsi_iser_ep_connect(struct Scsi_Host *shost, > struct iser_conn *iser_conn; > struct iscsi_endpoint *ep; > > - ep = iscsi_create_endpoint(0); > + ep = iscsi_create_endpoint(shost, 0); > if (!ep) > return ERR_PTR(-ENOMEM); > > diff --git a/drivers/scsi/be2iscsi/be_iscsi.c b/drivers/scsi/be2iscsi/be_iscsi.c > index 8aeaddc93b16..c893d193f5ef 100644 > --- a/drivers/scsi/be2iscsi/be_iscsi.c > +++ b/drivers/scsi/be2iscsi/be_iscsi.c > @@ -1168,7 +1168,7 @@ beiscsi_ep_connect(struct Scsi_Host *shost, struct sockaddr *dst_addr, > return ERR_PTR(ret); > } > > - ep = iscsi_create_endpoint(sizeof(struct beiscsi_endpoint)); > + ep = iscsi_create_endpoint(shost, sizeof(struct beiscsi_endpoint)); > if (!ep) { > ret = -ENOMEM; > return ERR_PTR(ret); > diff --git a/drivers/scsi/bnx2i/bnx2i_iscsi.c b/drivers/scsi/bnx2i/bnx2i_iscsi.c > index a3c800e04a2e..ac63e93e07c6 100644 > --- a/drivers/scsi/bnx2i/bnx2i_iscsi.c > +++ b/drivers/scsi/bnx2i/bnx2i_iscsi.c > @@ -384,7 +384,7 @@ static struct iscsi_endpoint *bnx2i_alloc_ep(struct bnx2i_hba *hba) > struct bnx2i_endpoint *bnx2i_ep; > u32 ec_div; > > - ep = iscsi_create_endpoint(sizeof(*bnx2i_ep)); > + ep = iscsi_create_endpoint(hba->shost, sizeof(*bnx2i_ep)); > if (!ep) { > printk(KERN_ERR "bnx2i: Could not allocate ep\n"); > return NULL; > diff --git a/drivers/scsi/cxgbi/libcxgbi.c b/drivers/scsi/cxgbi/libcxgbi.c > index af281e271f88..94edf8e1fb0c 100644 > --- a/drivers/scsi/cxgbi/libcxgbi.c > +++ b/drivers/scsi/cxgbi/libcxgbi.c > @@ -2926,7 +2926,7 @@ struct iscsi_endpoint *cxgbi_ep_connect(struct Scsi_Host *shost, > goto release_conn; > } > > - ep = iscsi_create_endpoint(sizeof(*cep)); > + ep = iscsi_create_endpoint(shost, sizeof(*cep)); > if (!ep) { > err = -ENOMEM; > pr_info("iscsi alloc ep, OOM.\n"); > diff --git a/drivers/scsi/qedi/qedi_iscsi.c b/drivers/scsi/qedi/qedi_iscsi.c > index 31ec429104e2..4baf1dbb8e92 100644 > --- a/drivers/scsi/qedi/qedi_iscsi.c > +++ b/drivers/scsi/qedi/qedi_iscsi.c > @@ -931,7 +931,7 @@ qedi_ep_connect(struct Scsi_Host *shost, struct sockaddr *dst_addr, > return ERR_PTR(-ENXIO); > } > > - ep = iscsi_create_endpoint(sizeof(struct qedi_endpoint)); > + ep = iscsi_create_endpoint(shost, sizeof(struct qedi_endpoint)); > if (!ep) { > QEDI_ERR(&qedi->dbg_ctx, "endpoint create fail\n"); > ret = -ENOMEM; > diff --git a/drivers/scsi/qla4xxx/ql4_os.c b/drivers/scsi/qla4xxx/ql4_os.c > index 005502125b27..acebf9c92c04 100644 > --- a/drivers/scsi/qla4xxx/ql4_os.c > +++ b/drivers/scsi/qla4xxx/ql4_os.c > @@ -1717,7 +1717,7 @@ qla4xxx_ep_connect(struct Scsi_Host *shost, struct sockaddr *dst_addr, > } > > ha = iscsi_host_priv(shost); > - ep = iscsi_create_endpoint(sizeof(struct qla_endpoint)); > + ep = iscsi_create_endpoint(shost, sizeof(struct qla_endpoint)); > if (!ep) { > ret = -ENOMEM; > return ERR_PTR(ret); > diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c > index be69cea9c6f8..86bafdb862a5 100644 > --- a/drivers/scsi/scsi_transport_iscsi.c > +++ b/drivers/scsi/scsi_transport_iscsi.c > @@ -204,7 +204,7 @@ static struct attribute_group iscsi_endpoint_group = { > }; > > struct iscsi_endpoint * > -iscsi_create_endpoint(int dd_size) > +iscsi_create_endpoint(struct Scsi_Host *shost, int dd_size) > { > struct iscsi_endpoint *ep; > int err, id; > @@ -230,6 +230,7 @@ iscsi_create_endpoint(int dd_size) > > ep->id = id; > ep->dev.class = &iscsi_endpoint_class; > + ep->dev.parent = &shost->shost_gendev; > dev_set_name(&ep->dev, "ep-%d", id); > err = device_register(&ep->dev); > if (err) Umm... doesn't this change the sysfs layout? IE won't the endpoint node be moved under the Scsi_Host directory? But even if it does: do we care? Cheers, Hannes
On Mar 14, 2023, at 9:23 AM, Hannes Reinecke <hare@suse.de> wrote: > > On 2/8/23 18:40, Lee Duncan wrote: >> From: Lee Duncan <lduncan@suse.com> >> Right now the iscsi_endpoint is only linked to a connection once that >> connection has been established. For net namespace filtering of the >> sysfs objects, associate an endpoint with the host that it was >> allocated for when it is created. >> Signed-off-by: Chris Leech <cleech@redhat.com> >> Signed-off-by: Lee Duncan <lduncan@suse.com> >> --- >> drivers/infiniband/ulp/iser/iscsi_iser.c | 2 +- >> drivers/scsi/be2iscsi/be_iscsi.c | 2 +- >> drivers/scsi/bnx2i/bnx2i_iscsi.c | 2 +- >> drivers/scsi/cxgbi/libcxgbi.c | 2 +- >> drivers/scsi/qedi/qedi_iscsi.c | 2 +- >> drivers/scsi/qla4xxx/ql4_os.c | 2 +- >> drivers/scsi/scsi_transport_iscsi.c | 3 ++- >> include/scsi/scsi_transport_iscsi.h | 6 +++++- >> 8 files changed, 13 insertions(+), 8 deletions(-) >> diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c b/drivers/infiniband/ulp/iser/iscsi_iser.c >> index 620ae5b2d80d..d38c909b462f 100644 >> --- a/drivers/infiniband/ulp/iser/iscsi_iser.c >> +++ b/drivers/infiniband/ulp/iser/iscsi_iser.c >> @@ -802,7 +802,7 @@ static struct iscsi_endpoint *iscsi_iser_ep_connect(struct Scsi_Host *shost, >> struct iser_conn *iser_conn; >> struct iscsi_endpoint *ep; >> - ep = iscsi_create_endpoint(0); >> + ep = iscsi_create_endpoint(shost, 0); >> if (!ep) >> return ERR_PTR(-ENOMEM); >> diff --git a/drivers/scsi/be2iscsi/be_iscsi.c b/drivers/scsi/be2iscsi/be_iscsi.c >> index 8aeaddc93b16..c893d193f5ef 100644 >> --- a/drivers/scsi/be2iscsi/be_iscsi.c >> +++ b/drivers/scsi/be2iscsi/be_iscsi.c >> @@ -1168,7 +1168,7 @@ beiscsi_ep_connect(struct Scsi_Host *shost, struct sockaddr *dst_addr, >> return ERR_PTR(ret); >> } >> - ep = iscsi_create_endpoint(sizeof(struct beiscsi_endpoint)); >> + ep = iscsi_create_endpoint(shost, sizeof(struct beiscsi_endpoint)); >> if (!ep) { >> ret = -ENOMEM; >> return ERR_PTR(ret); >> diff --git a/drivers/scsi/bnx2i/bnx2i_iscsi.c b/drivers/scsi/bnx2i/bnx2i_iscsi.c >> index a3c800e04a2e..ac63e93e07c6 100644 >> --- a/drivers/scsi/bnx2i/bnx2i_iscsi.c >> +++ b/drivers/scsi/bnx2i/bnx2i_iscsi.c >> @@ -384,7 +384,7 @@ static struct iscsi_endpoint *bnx2i_alloc_ep(struct bnx2i_hba *hba) >> struct bnx2i_endpoint *bnx2i_ep; >> u32 ec_div; >> - ep = iscsi_create_endpoint(sizeof(*bnx2i_ep)); >> + ep = iscsi_create_endpoint(hba->shost, sizeof(*bnx2i_ep)); >> if (!ep) { >> printk(KERN_ERR "bnx2i: Could not allocate ep\n"); >> return NULL; >> diff --git a/drivers/scsi/cxgbi/libcxgbi.c b/drivers/scsi/cxgbi/libcxgbi.c >> index af281e271f88..94edf8e1fb0c 100644 >> --- a/drivers/scsi/cxgbi/libcxgbi.c >> +++ b/drivers/scsi/cxgbi/libcxgbi.c >> @@ -2926,7 +2926,7 @@ struct iscsi_endpoint *cxgbi_ep_connect(struct Scsi_Host *shost, >> goto release_conn; >> } >> - ep = iscsi_create_endpoint(sizeof(*cep)); >> + ep = iscsi_create_endpoint(shost, sizeof(*cep)); >> if (!ep) { >> err = -ENOMEM; >> pr_info("iscsi alloc ep, OOM.\n"); >> diff --git a/drivers/scsi/qedi/qedi_iscsi.c b/drivers/scsi/qedi/qedi_iscsi.c >> index 31ec429104e2..4baf1dbb8e92 100644 >> --- a/drivers/scsi/qedi/qedi_iscsi.c >> +++ b/drivers/scsi/qedi/qedi_iscsi.c >> @@ -931,7 +931,7 @@ qedi_ep_connect(struct Scsi_Host *shost, struct sockaddr *dst_addr, >> return ERR_PTR(-ENXIO); >> } >> - ep = iscsi_create_endpoint(sizeof(struct qedi_endpoint)); >> + ep = iscsi_create_endpoint(shost, sizeof(struct qedi_endpoint)); >> if (!ep) { >> QEDI_ERR(&qedi->dbg_ctx, "endpoint create fail\n"); >> ret = -ENOMEM; >> diff --git a/drivers/scsi/qla4xxx/ql4_os.c b/drivers/scsi/qla4xxx/ql4_os.c >> index 005502125b27..acebf9c92c04 100644 >> --- a/drivers/scsi/qla4xxx/ql4_os.c >> +++ b/drivers/scsi/qla4xxx/ql4_os.c >> @@ -1717,7 +1717,7 @@ qla4xxx_ep_connect(struct Scsi_Host *shost, struct sockaddr *dst_addr, >> } >> ha = iscsi_host_priv(shost); >> - ep = iscsi_create_endpoint(sizeof(struct qla_endpoint)); >> + ep = iscsi_create_endpoint(shost, sizeof(struct qla_endpoint)); >> if (!ep) { >> ret = -ENOMEM; >> return ERR_PTR(ret); >> diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c >> index be69cea9c6f8..86bafdb862a5 100644 >> --- a/drivers/scsi/scsi_transport_iscsi.c >> +++ b/drivers/scsi/scsi_transport_iscsi.c >> @@ -204,7 +204,7 @@ static struct attribute_group iscsi_endpoint_group = { >> }; >> struct iscsi_endpoint * >> -iscsi_create_endpoint(int dd_size) >> +iscsi_create_endpoint(struct Scsi_Host *shost, int dd_size) >> { >> struct iscsi_endpoint *ep; >> int err, id; >> @@ -230,6 +230,7 @@ iscsi_create_endpoint(int dd_size) >> ep->id = id; >> ep->dev.class = &iscsi_endpoint_class; >> + ep->dev.parent = &shost->shost_gendev; >> dev_set_name(&ep->dev, "ep-%d", id); >> err = device_register(&ep->dev); >> if (err) > > Umm... doesn't this change the sysfs layout? > IE won't the endpoint node be moved under the Scsi_Host directory? > > But even if it does: do we care? > > > Cheers, > > Hannes > No, it’s still /sys/class/iscsi_endpoint, since the dev.class hasn’t changed. — Lee
On Tue, Mar 14, 2023 at 05:23:26PM +0100, Hannes Reinecke wrote: > On 2/8/23 18:40, Lee Duncan wrote: > > From: Lee Duncan <lduncan@suse.com> > > @@ -230,6 +230,7 @@ iscsi_create_endpoint(int dd_size) > > ep->id = id; > > ep->dev.class = &iscsi_endpoint_class; > > + ep->dev.parent = &shost->shost_gendev; > > dev_set_name(&ep->dev, "ep-%d", id); > > err = device_register(&ep->dev); > > if (err) > > Umm... doesn't this change the sysfs layout? > IE won't the endpoint node be moved under the Scsi_Host directory? > > But even if it does: do we care? It does, but it shouldn't matter. The Open-iSCSI tools look under the subsystem, not the device path. Being a child of the host makes more sense then being a floating virtual device. I just re-tested with bnx2i to make sure moving an endpoint devpath in sysfs didn't break anything. - Chris
On Wed, Feb 08, 2023 at 09:40:50AM -0800, Lee Duncan wrote: > Right now the iscsi_endpoint is only linked to a connection once that > connection has been established. For net namespace filtering of the > sysfs objects, associate an endpoint with the host that it was > allocated for when it is created. > > Signed-off-by: Chris Leech <cleech@redhat.com> > Signed-off-by: Lee Duncan <lduncan@suse.com> > --- > > diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c b/drivers/infiniband/ulp/iser/iscsi_iser.c > index 6b7603765383..212fa7aa9810 100644 > --- a/drivers/infiniband/ulp/iser/iscsi_iser.c > +++ b/drivers/infiniband/ulp/iser/iscsi_iser.c > @@ -802,7 +802,7 @@ static struct iscsi_endpoint *iscsi_iser_ep_connect(struct Scsi_Host *shost, > struct iser_conn *iser_conn; > struct iscsi_endpoint *ep; > > - ep = iscsi_create_endpoint(0); > + ep = iscsi_create_endpoint(shost, 0); > if (!ep) > return ERR_PTR(-ENOMEM); I started trying[1] to look at iSER, and I think this is a problem. iSER is the only iSCSI driver that uses endpoint objects, but does not require then to be bound to a host. That means that iscsi_iser_ep_connect can be called with a null shost. So this fails, and not in a new namespace. It just breaks iSER entirely. I think we need to preserve support for the iscsi_endpoint device having a virtual device path for iSER. Also, enabling net namespace support for iSER might require the ability to create an endpoint directly in a namespace instead of on a host. Kind of like the create_session discussion for iscsi_tcp. - Chris [1] I say trying, becuase before going and borrowing an RDMA setup I thought I'd give the kernel target and either siw or rxe a try. The isert module seems to have issues with siw, and I think maybe any iWARP, where setting enable_iser on a port will try and re-use the TCP port number and fail due to it being in use. With rxe my host failed, but that's becuase of this create_endpoint issue.
diff --git a/drivers/infiniband/ulp/iser/iscsi_iser.c b/drivers/infiniband/ulp/iser/iscsi_iser.c index 620ae5b2d80d..d38c909b462f 100644 --- a/drivers/infiniband/ulp/iser/iscsi_iser.c +++ b/drivers/infiniband/ulp/iser/iscsi_iser.c @@ -802,7 +802,7 @@ static struct iscsi_endpoint *iscsi_iser_ep_connect(struct Scsi_Host *shost, struct iser_conn *iser_conn; struct iscsi_endpoint *ep; - ep = iscsi_create_endpoint(0); + ep = iscsi_create_endpoint(shost, 0); if (!ep) return ERR_PTR(-ENOMEM); diff --git a/drivers/scsi/be2iscsi/be_iscsi.c b/drivers/scsi/be2iscsi/be_iscsi.c index 8aeaddc93b16..c893d193f5ef 100644 --- a/drivers/scsi/be2iscsi/be_iscsi.c +++ b/drivers/scsi/be2iscsi/be_iscsi.c @@ -1168,7 +1168,7 @@ beiscsi_ep_connect(struct Scsi_Host *shost, struct sockaddr *dst_addr, return ERR_PTR(ret); } - ep = iscsi_create_endpoint(sizeof(struct beiscsi_endpoint)); + ep = iscsi_create_endpoint(shost, sizeof(struct beiscsi_endpoint)); if (!ep) { ret = -ENOMEM; return ERR_PTR(ret); diff --git a/drivers/scsi/bnx2i/bnx2i_iscsi.c b/drivers/scsi/bnx2i/bnx2i_iscsi.c index a3c800e04a2e..ac63e93e07c6 100644 --- a/drivers/scsi/bnx2i/bnx2i_iscsi.c +++ b/drivers/scsi/bnx2i/bnx2i_iscsi.c @@ -384,7 +384,7 @@ static struct iscsi_endpoint *bnx2i_alloc_ep(struct bnx2i_hba *hba) struct bnx2i_endpoint *bnx2i_ep; u32 ec_div; - ep = iscsi_create_endpoint(sizeof(*bnx2i_ep)); + ep = iscsi_create_endpoint(hba->shost, sizeof(*bnx2i_ep)); if (!ep) { printk(KERN_ERR "bnx2i: Could not allocate ep\n"); return NULL; diff --git a/drivers/scsi/cxgbi/libcxgbi.c b/drivers/scsi/cxgbi/libcxgbi.c index af281e271f88..94edf8e1fb0c 100644 --- a/drivers/scsi/cxgbi/libcxgbi.c +++ b/drivers/scsi/cxgbi/libcxgbi.c @@ -2926,7 +2926,7 @@ struct iscsi_endpoint *cxgbi_ep_connect(struct Scsi_Host *shost, goto release_conn; } - ep = iscsi_create_endpoint(sizeof(*cep)); + ep = iscsi_create_endpoint(shost, sizeof(*cep)); if (!ep) { err = -ENOMEM; pr_info("iscsi alloc ep, OOM.\n"); diff --git a/drivers/scsi/qedi/qedi_iscsi.c b/drivers/scsi/qedi/qedi_iscsi.c index 31ec429104e2..4baf1dbb8e92 100644 --- a/drivers/scsi/qedi/qedi_iscsi.c +++ b/drivers/scsi/qedi/qedi_iscsi.c @@ -931,7 +931,7 @@ qedi_ep_connect(struct Scsi_Host *shost, struct sockaddr *dst_addr, return ERR_PTR(-ENXIO); } - ep = iscsi_create_endpoint(sizeof(struct qedi_endpoint)); + ep = iscsi_create_endpoint(shost, sizeof(struct qedi_endpoint)); if (!ep) { QEDI_ERR(&qedi->dbg_ctx, "endpoint create fail\n"); ret = -ENOMEM; diff --git a/drivers/scsi/qla4xxx/ql4_os.c b/drivers/scsi/qla4xxx/ql4_os.c index 005502125b27..acebf9c92c04 100644 --- a/drivers/scsi/qla4xxx/ql4_os.c +++ b/drivers/scsi/qla4xxx/ql4_os.c @@ -1717,7 +1717,7 @@ qla4xxx_ep_connect(struct Scsi_Host *shost, struct sockaddr *dst_addr, } ha = iscsi_host_priv(shost); - ep = iscsi_create_endpoint(sizeof(struct qla_endpoint)); + ep = iscsi_create_endpoint(shost, sizeof(struct qla_endpoint)); if (!ep) { ret = -ENOMEM; return ERR_PTR(ret); diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c index be69cea9c6f8..86bafdb862a5 100644 --- a/drivers/scsi/scsi_transport_iscsi.c +++ b/drivers/scsi/scsi_transport_iscsi.c @@ -204,7 +204,7 @@ static struct attribute_group iscsi_endpoint_group = { }; struct iscsi_endpoint * -iscsi_create_endpoint(int dd_size) +iscsi_create_endpoint(struct Scsi_Host *shost, int dd_size) { struct iscsi_endpoint *ep; int err, id; @@ -230,6 +230,7 @@ iscsi_create_endpoint(int dd_size) ep->id = id; ep->dev.class = &iscsi_endpoint_class; + ep->dev.parent = &shost->shost_gendev; dev_set_name(&ep->dev, "ep-%d", id); err = device_register(&ep->dev); if (err) diff --git a/include/scsi/scsi_transport_iscsi.h b/include/scsi/scsi_transport_iscsi.h index 34c03707fb6e..268ccaac1c05 100644 --- a/include/scsi/scsi_transport_iscsi.h +++ b/include/scsi/scsi_transport_iscsi.h @@ -287,6 +287,9 @@ struct iscsi_cls_session { #define iscsi_session_to_shost(_session) \ dev_to_shost(_session->dev.parent) +#define iscsi_endpoint_to_shost(_ep) \ + dev_to_shost(_ep->dev.parent) + #define starget_to_session(_stgt) \ iscsi_dev_to_session(_stgt->dev.parent) @@ -462,7 +465,8 @@ extern void iscsi_put_conn(struct iscsi_cls_conn *conn); extern void iscsi_get_conn(struct iscsi_cls_conn *conn); extern void iscsi_unblock_session(struct iscsi_cls_session *session); extern void iscsi_block_session(struct iscsi_cls_session *session); -extern struct iscsi_endpoint *iscsi_create_endpoint(int dd_size); +extern struct iscsi_endpoint *iscsi_create_endpoint(struct Scsi_Host *shost, + int dd_size); extern void iscsi_destroy_endpoint(struct iscsi_endpoint *ep); extern struct iscsi_endpoint *iscsi_lookup_endpoint(u64 handle); extern void iscsi_put_endpoint(struct iscsi_endpoint *ep);