Re: [PATCH] scsi: iscsi: Switch to using the new API kobj_to_dev()

2021-03-07 Thread James Bottomley 
On Mon, 2021-03-08 at 11:34 +0800, Jiapeng Chong wrote:
> Fix the following coccicheck warnings:
> 
> ./drivers/scsi/scsi_transport_iscsi.c:930:60-61: WARNING opportunity
> for kobj_to_dev().

I have to ask, what is the point of this?  container_of is usually
pretty safe ... as in it will detect when you screw up the usage.  The
only real misuse you can get is when the input type has an object of
the same name and return type and you got confused between two objects
with this property, but misuses like this resulting in bugs are very,
very rare.

Usually we wrap container_of because the wrapping is a bit shorter as
you can see: kobj_to_dev is about half the size of the container_of
form ... but is there any other reason to do it?

The problem is that container_of is a standard way of doing cast outs
in the kernel and we have hundreds of them.  To be precise, in scsi
alone:

jejb@jarvis:~/git/linux/drivers/scsi> git grep container_of|wc -l
496

So we really don't want to encourage wrapping them all because the
churn would be unbelievable and the gain minute.  So why should this
one case especially be wrapped when we don't want to wrap the others?

James


-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/open-iscsi/2b90f003bbf8064c2372cba6a61b31cb8dec7a69.camel%40linux.ibm.com.

Re: [PATCH] Check sk before sendpage

2019-07-10 Thread James Bottomley 
On Wed, 2019-07-10 at 17:47 +, Lee Duncan wrote:
> On 7/10/19 12:30 AM, Yang Bin wrote:
> 
> > From: " Yang Bin "
> > 
> > Before xmit,iscsi may disconnect just now.
> > So must check connection sock NULL or not,or kernel will crash for
> > accessing NULL pointer.
> > 
> > Signed-off-by: Yang Bin 
> > ---
> >  drivers/scsi/iscsi_tcp.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/scsi/iscsi_tcp.c b/drivers/scsi/iscsi_tcp.c
> > index 7bedbe8..a59c49f 100644
> > --- a/drivers/scsi/iscsi_tcp.c
> > +++ b/drivers/scsi/iscsi_tcp.c
> > @@ -264,6 +264,9 @@ static int iscsi_sw_tcp_xmit_segment(struct
> > iscsi_tcp_conn *tcp_conn,
> > unsigned int copied = 0;
> > int r = 0;
> >  
> > +   if (!sk)
> > +   return -ENOTCONN;
> > +
> > while (!iscsi_tcp_segment_done(tcp_conn, segment, 0, r)) {
> > struct scatterlist *sg;
> > unsigned int offset, copy;
> > 
> 
> If the socket can be closed right before iscsi_sw_tcp_xmit_segment()
> is called, can it be called in the middle of sending segments? (In
> which case the check would have to be in the while loop.)

I think the important point is: is this an actual observed bug or just
a theoretical problem?

The reason for asking is this call is controlled directly by the
ISCSI_UEVENT_DESTROY_CONN event sent by the iscsi daemon.  Obviously if
the daemon goes haywire and doesn't shut down the connection before
sending the destroy event, we may get the crash, but I would be
inclined to say fix the daemon.

James

Re: network namespace, netlink and sysfs changes for iSCSI (Re: [PATCH 0/9] use network namespace for iSCSI control interfaces)

2017-11-14 Thread James Bottomley 
On Tue, 2017-11-07 at 10:01 -0800, Chris Leech wrote:
> Hello,
> 
> I have this set of changes to the iSCSI control interfaces pending
> review, but seeing as it's sysfs and netlink changes there's not a
> lot of feedback from linux-scsi.

Well, it's a bit unlikely that they understand network namespaces.

> I was hoping I could get a brief review on the adding of network
> namespace support here.

Most network namespace work is done on net...@vger.kernel.org, but they
probably won't understand all the SCSI bits, but perhaps they don't
need to; it's basically the netlink and the filters, right?

The other list that would take a more generic view is the containers
one contain...@lists.linux-foundation.org because that's where most
namespace stuff ends up.

James



> Thank you,
> Chris
> 
> On Tue, Oct 31, 2017 at 03:40:55PM -0700, Chris Leech wrote:
> > 
> > This series of changes makes the iSCSI netlink and sysfs control
> > interfaces filtered by network namespace.  This is required to run
> > iscsid in any network namespace other than the initial default one.
> > 
> > Currently the netlink communication will fail if iscsid is started
> > in a non-default network namespace, as there is no kernel side
> > socket.  After fixing that, the rest of these changes are to filter
> > visibility of the iSCSI transport objects by netns.  This allows
> > for multiple iscsid instances to be run, one per netns, each
> > controlling it's own set of iSCSI sessions.
> > 
> > The iSCSI transport objects are filtered, but not the SCSI or block
> > layer devices.  So while iSCSI hosts and sessions become limited to
> > a network namespace, any attached devices remain visible system
> > wide.
> > 
> > This currently only supports iscsi_tcp running in a new namespace,
> > as it creates a virtual host per session.  Support could be added
> > later to allow assignment of iSCSI HBAs to network namespace, much
> > as is done for network interfaces.
> > 
> > Chris Leech (9):
> >   iscsi: create per-net iscsi netlink kernel sockets
> >   iscsi: associate endpoints with a host
> >   iscsi: sysfs filtering by network namespace
> >   iscsi: make all iSCSI netlink multicast namespace aware
> >   iscsi: set netns for iscsi_tcp hosts
> >   iscsi: check net namespace for all iscsi lookups
> >   iscsi: convert flashnode devices from bus to class
> >   iscsi: rename iscsi_bus_flash_* to iscsi_flash_*
> >   iscsi: filter flashnode sysfs by net namespace
> > 
> >  drivers/infiniband/ulp/iser/iscsi_iser.c |   7 +-
> >  drivers/scsi/be2iscsi/be_iscsi.c |   6 +-
> >  drivers/scsi/bnx2i/bnx2i_iscsi.c |   6 +-
> >  drivers/scsi/cxgbi/libcxgbi.c|   6 +-
> >  drivers/scsi/iscsi_tcp.c |   7 +
> >  drivers/scsi/qedi/qedi_iscsi.c   |   6 +-
> >  drivers/scsi/qla4xxx/ql4_os.c|  62 +--
> >  drivers/scsi/scsi_transport_iscsi.c  | 625
> > ++-
> >  include/scsi/scsi_transport_iscsi.h  |  63 ++--
> >  9 files changed, 538 insertions(+), 250 deletions(-)
> > 
> > -- 
> > 2.9.5
> > 
> 

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To post to this group, send email to open-iscsi@googlegroups.com.
Visit this group at https://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH] Use ida_simple for SCSI iSCSI transport session id

2016-02-12 Thread James Bottomley 
On Fri, 2016-02-12 at 09:38 -0800, Lee Duncan wrote:
> The scsi_transport_iscsi module already uses the ida_simple
> routines for managing the target ID, if requested to do
> so. This change replaces an ever-increasing atomic integer
> that tracks the session ID itself with the ida_simple
> family of routines. This means that the session ID
> will be reclaimed and can be reused when the session
> is freed.

Is reusing session ID's really a good idea?  For sequential sessions it
means that the ID of the next session will be re-used, i.e. the same as
the previous sessions, which could lead to target confusion.  I think
local uniqueness of session IDs is more important than wrap around
because sessions are short lived entities and the chances of the same
session being alive by the time we've wrapped is pretty tiny.

If you can demostrate a multi-target problem, perhaps we should rather
fix this by making the next session id a target local quantity?

James

-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To post to this group, send email to open-iscsi@googlegroups.com.
Visit this group at https://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.

Re: DEFINE_IDA causing memory leaks? (was Re: [PATCH 1/2] virtio: fix memory leak of virtio ida cache layers)

2015-09-17 Thread James Bottomley 
On Thu, 2015-09-17 at 08:33 +0300, Michael S. Tsirkin wrote:
> On Wed, Sep 16, 2015 at 07:29:17PM -0500, Suman Anna wrote:
> > The virtio core uses a static ida named virtio_index_ida for
> > assigning index numbers to virtio devices during registration.
> > The ida core may allocate some internal idr cache layers and
> > an ida bitmap upon any ida allocation, and all these layers are
> > truely freed only upon the ida destruction. The virtio_index_ida
> > is not destroyed at present, leading to a memory leak when using
> > the virtio core as a module and atleast one virtio device is
> > registered and unregistered.
> > 
> > Fix this by invoking ida_destroy() in the virtio core module
> > exit.
> > 
> > Cc: "Michael S. Tsirkin" 
> > Signed-off-by: Suman Anna 
> 
> Interesting.
> Will the same apply to e.g. sd_index_ida in drivers/scsi/sd.c
> or iscsi_sess_ida in drivers/scsi/scsi_transport_iscsi.c?
> 
> If no, why not?
> 
> One doesn't generally expect to have to free global variables.
> Maybe we should forbid DEFINE_IDA in modules?
> 
> James, could you comment on this please?

ida is Tejun's baby (cc'd).  However, it does look like without
ida_destroy() you will leave a cached ida->bitmap dangling because we're
trying to be a bit clever in ida_remove() so we cache the bitmap to
relieve ida_pre_get() of the burden if we would otherwise free it.

I don't understand why you'd want to forbid DEFINE_IDA ... all it does
is pre-initialise a usually static ida structure.  The initialised
structure will have a NULL bitmap cache that's allocated in the first
ida_pre_get() ... that all seems to work as expected and no different
from a dynamically allocated struct ida.  Or are you thinking because
ida_destory() doesn't set bitmap to NULL, it damages the reuse?  In
which case I'm not sure there's much benefit to making it reusable, but
I suppose we could by adding a memset into ida_destroy().

James

> > ---
> >  drivers/virtio/virtio.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > index b1877d73fa56..7062bb0975a5 100644
> > --- a/drivers/virtio/virtio.c
> > +++ b/drivers/virtio/virtio.c
> > @@ -412,6 +412,7 @@ static int virtio_init(void)
> >  static void __exit virtio_exit(void)
> >  {
> > bus_unregister(_bus);
> > +   ida_destroy(_index_ida);
> >  }
> >  core_initcall(virtio_init);
> >  module_exit(virtio_exit);
> > -- 
> > 2.5.0
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 




-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To post to this group, send email to open-iscsi@googlegroups.com.
Visit this group at http://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.

Re: DEFINE_IDA causing memory leaks? (was Re: [PATCH 1/2] virtio: fix memory leak of virtio ida cache layers)

2015-09-17 Thread James Bottomley 
On Thu, 2015-09-17 at 19:06 +0300, Michael S. Tsirkin wrote:
> On Thu, Sep 17, 2015 at 07:15:44AM -0700, James Bottomley wrote:
> > On Thu, 2015-09-17 at 08:33 +0300, Michael S. Tsirkin wrote:
> > > On Wed, Sep 16, 2015 at 07:29:17PM -0500, Suman Anna wrote:
> > > > The virtio core uses a static ida named virtio_index_ida for
> > > > assigning index numbers to virtio devices during registration.
> > > > The ida core may allocate some internal idr cache layers and
> > > > an ida bitmap upon any ida allocation, and all these layers are
> > > > truely freed only upon the ida destruction. The virtio_index_ida
> > > > is not destroyed at present, leading to a memory leak when using
> > > > the virtio core as a module and atleast one virtio device is
> > > > registered and unregistered.
> > > > 
> > > > Fix this by invoking ida_destroy() in the virtio core module
> > > > exit.
> > > > 
> > > > Cc: "Michael S. Tsirkin" <m...@redhat.com>
> > > > Signed-off-by: Suman Anna <s-a...@ti.com>
> > > 
> > > Interesting.
> > > Will the same apply to e.g. sd_index_ida in drivers/scsi/sd.c
> > > or iscsi_sess_ida in drivers/scsi/scsi_transport_iscsi.c?
> > > 
> > > If no, why not?
> > > 
> > > One doesn't generally expect to have to free global variables.
> > > Maybe we should forbid DEFINE_IDA in modules?
> > > 
> > > James, could you comment on this please?
> > 
> > ida is Tejun's baby (cc'd).  However, it does look like without
> > ida_destroy() you will leave a cached ida->bitmap dangling because we're
> > trying to be a bit clever in ida_remove() so we cache the bitmap to
> > relieve ida_pre_get() of the burden if we would otherwise free it.
> > 
> > I don't understand why you'd want to forbid DEFINE_IDA ... all it does
> > is pre-initialise a usually static ida structure.  The initialised
> > structure will have a NULL bitmap cache that's allocated in the first
> > ida_pre_get() ... that all seems to work as expected and no different
> > from a dynamically allocated struct ida.  Or are you thinking because
> > ida_destory() doesn't set bitmap to NULL, it damages the reuse?  In
> > which case I'm not sure there's much benefit to making it reusable, but
> > I suppose we could by adding a memset into ida_destroy().
> > 
> > James
> 
> It's just unusual to have  a descructor without a constructor.
> I bet more drivers misuse this AI because of this.


Well, there's an easy fix for that.  We could have ida_remove() actually
free the bitmap and not cache it if it's the last layer.  That way ida
would naturally empty and we wouldn't need a destructor.   Tejun, would
that work?

James

> > > > ---
> > > >  drivers/virtio/virtio.c | 1 +
> > > >  1 file changed, 1 insertion(+)
> > > > 
> > > > diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
> > > > index b1877d73fa56..7062bb0975a5 100644
> > > > --- a/drivers/virtio/virtio.c
> > > > +++ b/drivers/virtio/virtio.c
> > > > @@ -412,6 +412,7 @@ static int virtio_init(void)
> > > >  static void __exit virtio_exit(void)
> > > >  {
> > > > bus_unregister(_bus);
> > > > +   ida_destroy(_index_ida);
> > > >  }
> > > >  core_initcall(virtio_init);
> > > >  module_exit(virtio_exit);
> > > > -- 
> > > > 2.5.0
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> > > the body of a message to majord...@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > 
> > 
> > 
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 



-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To post to this group, send email to open-iscsi@googlegroups.com.
Visit this group at http://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.

Re: DEFINE_IDA causing memory leaks? (was Re: [PATCH 1/2] virtio: fix memory leak of virtio ida cache layers)

2015-09-17 Thread James Bottomley 
On Thu, 2015-09-17 at 13:15 -0400, Tejun Heo wrote:
> Hello,
> 
> On Thu, Sep 17, 2015 at 09:48:37AM -0700, James Bottomley wrote:
> > Well, there's an easy fix for that.  We could have ida_remove() actually
> > free the bitmap and not cache it if it's the last layer.  That way ida
> > would naturally empty and we wouldn't need a destructor.   Tejun, would
> > that work?
> 
> Yeah, that definitely is one way to go about it.  It kinda muddles the
> purpose of ida_destroy() tho.  I suppose we can rename it to
> idr_remove_all() and then do the same to idr.  I'm not particularly
> objecting to all that but what's wrong with just calling idr_destroy()
> on exit paths?  If missing the call in modules is an issue, maybe we
> can just annotate idr/ida with debugobj?

The argument is that we shouldn't have to explicitly destroy a
statically initialized object, so 

DEFINE_IDA(someida);

Should just work without having to explicitly do

ida_destory(someida);

somewhere in the exit code.  It's about usage patterns.  Michael's
argument is that if we can't follow the no destructor pattern for
DEFINE_IDA() then we shouldn't have it at all, because it's confusing
kernel design patterns.  The pattern we would have would be

struct ida someida:

ida_init();

...

ida_destroy();

so the object explicitly has a constructor matched to a destructor.

James


-- 
You received this message because you are subscribed to the Google Groups 
"open-iscsi" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To post to this group, send email to open-iscsi@googlegroups.com.
Visit this group at http://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.

Re: [Lsf-pc] [LSF/MM TOPIC] iSCSI MQ adoption via MCS discussion

2015-01-09 Thread James Bottomley 
On Fri, 2015-01-09 at 19:28 +0100, Hannes Reinecke wrote:
[...]
  I think you are assuming we are leaving the iscsi code as it is today.
  
  For the non-MCS mq session per CPU design, we would be allocating and
  binding the session and its resources to specific CPUs. They would only
  be accessed by the threads on that one CPU, so we get our
  serialization/synchronization from that. That is why we are saying we
  do not need something like atomic_t/spin_locks for the sequence number
  handling for this type of implementation.
  
 Wouldn't that need to be coordinated with the networking layer?
 Doesn't it do the same thing, matching TX/RX queues to CPUs?
 If so, wouldn't we decrease bandwidth by restricting things to one CPU?

So this is actually one of the fascinating questions on multi-queue.
Long ago, when I worked for the NCR OS group and we were bringing up the
first SMP systems, we actually found that the SCSI stack went faster
when bound to a single CPU.  The problem in those days was lock
granularity and contention, so single CPU binding eliminated that
overhead.  However, nowadays with modern multi-tiered caching and huge
latencies for cache line bouncing, we're approaching the point where the
fineness of our lock granularity is hurting performance, so it's worth
re-asking the question of whether just dumping all the lock latency by
single CPU binding is a worthwhile exercise.

James

-- 
You received this message because you are subscribed to the Google Groups 
open-iscsi group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To post to this group, send email to open-iscsi@googlegroups.com.
Visit this group at http://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.

Re: [Lsf-pc] [LSF/MM TOPIC] iSCSI MQ adoption via MCS discussion

2015-01-08 Thread James Bottomley 
On Thu, 2015-01-08 at 14:57 -0800, Nicholas A. Bellinger wrote:
 On Thu, 2015-01-08 at 14:29 -0800, James Bottomley wrote:
  On Thu, 2015-01-08 at 14:16 -0800, Nicholas A. Bellinger wrote:
   On Thu, 2015-01-08 at 08:50 +0100, Bart Van Assche wrote:
On 01/07/15 22:39, Mike Christie wrote:
 On 01/07/2015 10:57 AM, Hannes Reinecke wrote:
 On 01/07/2015 05:25 PM, Sagi Grimberg wrote:
 Hi everyone,

 Now that scsi-mq is fully included, we need an iSCSI initiator that
 would use it to achieve scalable performance. The need is even 
 greater
 for iSCSI offload devices and transports that support multiple HW
 queues. As iSER maintainer I'd like to discuss the way we would 
 choose
 to implement that in iSCSI.

 My measurements show that iSER initiator can scale up to ~2.1M IOPs
 with multiple sessions but only ~630K IOPs with a single session 
 where
 the most significant bottleneck the (single) core processing
 completions.

 In the existing single connection per session model, given that 
 command
 ordering must be preserved session-wide, we end up in a serial 
 command
 execution over a single connection which is basically a single queue
 model. The best fit seems to be plugging iSCSI MCS as a multi-queued
 scsi LLDD. In this model, a hardware context will have a 1x1 mapping
 with an iSCSI connection (TCP socket or a HW queue).

 iSCSI MCS and it's role in the presence of dm-multipath layer was
 discussed several times in the past decade(s). The basic need for 
 MCS is
 implementing a multi-queue data path, so perhaps we may want to 
 avoid
 doing any type link aggregation or load balancing to not overlap
 dm-multipath. For example we can implement ERL=0 (which is 
 basically the
 scsi-mq ERL) and/or restrict a session to a single portal.

 As I see it, the todo's are:
 1. Getting MCS to work (kernel + user-space) with ERL=0 and a
 round-robin connection selection (per scsi command execution).
 2. Plug into scsi-mq - exposing num_connections as nr_hw_queues and
 using blk-mq based queue (conn) selection.
 3. Rework iSCSI core locking scheme to avoid session-wide locking
 as much as possible.
 4. Use blk-mq pre-allocation and tagging facilities.

 I've recently started looking into this. I would like the community 
 to
 agree (or debate) on this scheme and also talk about implementation
 with anyone who is also interested in this.

 Yes, that's a really good topic.

 I've pondered implementing MC/S for iscsi/TCP but then I've figured 
 my
 network implementation knowledge doesn't spread that far.
 So yeah, a discussion here would be good.

 Mike? Any comments?

 I have been working under the assumption that people would be ok with
 MCS upstream if we are only using it to handle the issue where we want
 to do something like have a tcp/iscsi connection per CPU then map the
 connection to a blk_mq_hw_ctx. In this more limited MCS implementation
 there would be no iscsi layer code to do something like load balance
 across ports or transport paths like how dm-multipath does, so there
 would be no feature/code duplication. For balancing across hctxs, then
 the iscsi layer would also leave that up to whatever we end up with in
 upper layers, so again no feature/code duplication with upper layers.

 So pretty non controversial I hope :)

 If people want to add something like round robin connection selection 
 in
 the iscsi layer, then I think we want to leave that for after the
 initial merge, so people can argue about that separately.

Hello Sagi and Mike,

I agree with Sagi that adding scsi-mq support in the iSER initiator 
would help iSER users because that would allow these users to configure 
a single iSER target and use the multiqueue feature instead of having 
to 
configure multiple iSER targets to spread the workload over multiple 
cpus at the target side.

And I agree with Mike that implementing scsi-mq support in the iSER 
initiator as multiple independent connections probably is a better 
choice than MC/S. RFC 3720 namely requires that iSCSI numbering is 
session-wide. This means maintaining a single counter for all MC/S 
sessions. Such a counter would be a contention point. I'm afraid that 
because of that counter performance on a multi-socket initiator system 
with a scsi-mq implementation based on MC/S could be worse than with 
the 
approach with multiple iSER targets. Hence my preference for an 
approach 
based on multiple independent iSER connections instead of MC/S.

   
   The idea that a simple session wide counter for command sequence number
   assignment adds such a degree of contention

Re: [Lsf-pc] [LSF/MM TOPIC] iSCSI MQ adoption via MCS discussion

2015-01-08 Thread James Bottomley 
On Thu, 2015-01-08 at 21:03 -0800, Nicholas A. Bellinger wrote:
 On Thu, 2015-01-08 at 15:22 -0800, James Bottomley wrote:
  On Thu, 2015-01-08 at 14:57 -0800, Nicholas A. Bellinger wrote:
   On Thu, 2015-01-08 at 14:29 -0800, James Bottomley wrote:
On Thu, 2015-01-08 at 14:16 -0800, Nicholas A. Bellinger wrote:
 
 SNIP
 
   The point is that a simple session wide counter for command sequence
   number assignment is significantly less overhead than all of the
   overhead associated with running a full multipath stack atop multiple
   sessions.
  
  I don't see how that's relevant to issue speed, which was the measure we
  were using: The layers above are just a hopper.  As long as they're
  loaded, the MQ lower layer can issue at full speed.  So as long as the
  multipath hopper is efficient enough to keep the queues loaded there's
  no speed degradation.
  
  The problem with a sequence point inside the MQ issue layer is that it
  can cause a stall that reduces the issue speed. so the counter sequence
  point causes a degraded issue speed over the multipath hopper approach
  above even if the multipath approach has a higher CPU overhead.
  
  Now, if the system is close to 100% cpu already, *then* the multipath
  overhead will try to take CPU power we don't have and cause a stall, but
  it's only in the flat out CPU case.
  
   Not to mention that our iSCSI/iSER initiator is already taking a session
   wide lock when sending outgoing PDUs, so adding a session wide counter
   isn't adding any additional synchronization overhead vs. what's already
   in place.
  
  I'll leave it up to the iSER people to decide whether they're redoing
  this as part of the MQ work.
  
 
 Session wide command sequence number synchronization isn't something to
 be removed as part of the MQ work.  It's a iSCSI/iSER protocol
 requirement.

The sequence number is a requirement of the session.  Multiple separate
sessions means no SN correlation between the different connections, so
no global requirement for a SN counter across the queues ... that's what
Mike was saying about implementing multipath not using MCS.  With MCS we
have a single session for all the queues and thus have to correlate the
sequence number across all the connections and hence all the queues;
without it we don't.  That's why the sequence number becomes a potential
stall point in MQ implementation of MCS which can be obviated if we use
a separate session per queue.

James


-- 
You received this message because you are subscribed to the Google Groups 
open-iscsi group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To post to this group, send email to open-iscsi@googlegroups.com.
Visit this group at http://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.

Re: [LSF/MM TOPIC] iSCSI MQ adoption via MCS discussion

2015-01-08 Thread James Bottomley 
On Thu, 2015-01-08 at 14:16 -0800, Nicholas A. Bellinger wrote:
 On Thu, 2015-01-08 at 08:50 +0100, Bart Van Assche wrote:
  On 01/07/15 22:39, Mike Christie wrote:
   On 01/07/2015 10:57 AM, Hannes Reinecke wrote:
   On 01/07/2015 05:25 PM, Sagi Grimberg wrote:
   Hi everyone,
  
   Now that scsi-mq is fully included, we need an iSCSI initiator that
   would use it to achieve scalable performance. The need is even greater
   for iSCSI offload devices and transports that support multiple HW
   queues. As iSER maintainer I'd like to discuss the way we would choose
   to implement that in iSCSI.
  
   My measurements show that iSER initiator can scale up to ~2.1M IOPs
   with multiple sessions but only ~630K IOPs with a single session where
   the most significant bottleneck the (single) core processing
   completions.
  
   In the existing single connection per session model, given that command
   ordering must be preserved session-wide, we end up in a serial command
   execution over a single connection which is basically a single queue
   model. The best fit seems to be plugging iSCSI MCS as a multi-queued
   scsi LLDD. In this model, a hardware context will have a 1x1 mapping
   with an iSCSI connection (TCP socket or a HW queue).
  
   iSCSI MCS and it's role in the presence of dm-multipath layer was
   discussed several times in the past decade(s). The basic need for MCS is
   implementing a multi-queue data path, so perhaps we may want to avoid
   doing any type link aggregation or load balancing to not overlap
   dm-multipath. For example we can implement ERL=0 (which is basically the
   scsi-mq ERL) and/or restrict a session to a single portal.
  
   As I see it, the todo's are:
   1. Getting MCS to work (kernel + user-space) with ERL=0 and a
   round-robin connection selection (per scsi command execution).
   2. Plug into scsi-mq - exposing num_connections as nr_hw_queues and
   using blk-mq based queue (conn) selection.
   3. Rework iSCSI core locking scheme to avoid session-wide locking
   as much as possible.
   4. Use blk-mq pre-allocation and tagging facilities.
  
   I've recently started looking into this. I would like the community to
   agree (or debate) on this scheme and also talk about implementation
   with anyone who is also interested in this.
  
   Yes, that's a really good topic.
  
   I've pondered implementing MC/S for iscsi/TCP but then I've figured my
   network implementation knowledge doesn't spread that far.
   So yeah, a discussion here would be good.
  
   Mike? Any comments?
  
   I have been working under the assumption that people would be ok with
   MCS upstream if we are only using it to handle the issue where we want
   to do something like have a tcp/iscsi connection per CPU then map the
   connection to a blk_mq_hw_ctx. In this more limited MCS implementation
   there would be no iscsi layer code to do something like load balance
   across ports or transport paths like how dm-multipath does, so there
   would be no feature/code duplication. For balancing across hctxs, then
   the iscsi layer would also leave that up to whatever we end up with in
   upper layers, so again no feature/code duplication with upper layers.
  
   So pretty non controversial I hope :)
  
   If people want to add something like round robin connection selection in
   the iscsi layer, then I think we want to leave that for after the
   initial merge, so people can argue about that separately.
  
  Hello Sagi and Mike,
  
  I agree with Sagi that adding scsi-mq support in the iSER initiator 
  would help iSER users because that would allow these users to configure 
  a single iSER target and use the multiqueue feature instead of having to 
  configure multiple iSER targets to spread the workload over multiple 
  cpus at the target side.
  
  And I agree with Mike that implementing scsi-mq support in the iSER 
  initiator as multiple independent connections probably is a better 
  choice than MC/S. RFC 3720 namely requires that iSCSI numbering is 
  session-wide. This means maintaining a single counter for all MC/S 
  sessions. Such a counter would be a contention point. I'm afraid that 
  because of that counter performance on a multi-socket initiator system 
  with a scsi-mq implementation based on MC/S could be worse than with the 
  approach with multiple iSER targets. Hence my preference for an approach 
  based on multiple independent iSER connections instead of MC/S.
  
 
 The idea that a simple session wide counter for command sequence number
 assignment adds such a degree of contention that it renders MC/S at a
 performance disadvantage vs. multi-session configurations with all of
 the extra multipath logic overhead on top is at best, a naive
 proposition.
 
 On the initiator side for MC/S, literally the only thing that needs to
 be serialized is the assignment of the command sequence number to
 individual non-immediate PDUs.  The sending of the outgoing PDUs +
 immediate

Re: [LSF/MM TOPIC] iSCSI MQ adoption via MCS discussion

2015-01-08 Thread James Bottomley 
On Wed, 2015-01-07 at 15:39 -0600, Mike Christie wrote:
 On 01/07/2015 10:57 AM, Hannes Reinecke wrote:
  On 01/07/2015 05:25 PM, Sagi Grimberg wrote:
  Hi everyone,
 
  Now that scsi-mq is fully included, we need an iSCSI initiator that
  would use it to achieve scalable performance. The need is even greater
  for iSCSI offload devices and transports that support multiple HW
  queues. As iSER maintainer I'd like to discuss the way we would choose
  to implement that in iSCSI.
 
  My measurements show that iSER initiator can scale up to ~2.1M IOPs
  with multiple sessions but only ~630K IOPs with a single session where
  the most significant bottleneck the (single) core processing
  completions.
 
  In the existing single connection per session model, given that command
  ordering must be preserved session-wide, we end up in a serial command
  execution over a single connection which is basically a single queue
  model. The best fit seems to be plugging iSCSI MCS as a multi-queued
  scsi LLDD. In this model, a hardware context will have a 1x1 mapping
  with an iSCSI connection (TCP socket or a HW queue).
 
  iSCSI MCS and it's role in the presence of dm-multipath layer was
  discussed several times in the past decade(s). The basic need for MCS is
  implementing a multi-queue data path, so perhaps we may want to avoid
  doing any type link aggregation or load balancing to not overlap
  dm-multipath. For example we can implement ERL=0 (which is basically the
  scsi-mq ERL) and/or restrict a session to a single portal.
 
  As I see it, the todo's are:
  1. Getting MCS to work (kernel + user-space) with ERL=0 and a
 round-robin connection selection (per scsi command execution).
  2. Plug into scsi-mq - exposing num_connections as nr_hw_queues and
 using blk-mq based queue (conn) selection.
  3. Rework iSCSI core locking scheme to avoid session-wide locking
 as much as possible.
  4. Use blk-mq pre-allocation and tagging facilities.
 
  I've recently started looking into this. I would like the community to
  agree (or debate) on this scheme and also talk about implementation
  with anyone who is also interested in this.
 
  Yes, that's a really good topic.
  
  I've pondered implementing MC/S for iscsi/TCP but then I've figured my
  network implementation knowledge doesn't spread that far.
  So yeah, a discussion here would be good.
  
  Mike? Any comments?
 
 I have been working under the assumption that people would be ok with
 MCS upstream if we are only using it to handle the issue where we want
 to do something like have a tcp/iscsi connection per CPU then map the
 connection to a blk_mq_hw_ctx. In this more limited MCS implementation
 there would be no iscsi layer code to do something like load balance
 across ports or transport paths like how dm-multipath does, so there
 would be no feature/code duplication. For balancing across hctxs, then
 the iscsi layer would also leave that up to whatever we end up with in
 upper layers, so again no feature/code duplication with upper layers.
 
 So pretty non controversial I hope :)

If you can make that work, so we expose MCS in a way that allows upper
layers to use it, I'd say it was pretty much perfect.  The main
objection I've had over the years to multiple connections per session is
that it required a duplication of the multi-path code within the iscsi
initiator (and that was after several long fights to get multi path out
of other fabric initiators), so something that doesn't require the
duplication overcomes that objection.

 If people want to add something like round robin connection selection in
 the iscsi layer, then I think we want to leave that for after the
 initial merge, so people can argue about that separately.

Well, you're right, we can argue about it later, but if it's just round
robin, why would it be better done in the initator rather than dm?

James


-- 
You received this message because you are subscribed to the Google Groups 
open-iscsi group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To post to this group, send email to open-iscsi@googlegroups.com.
Visit this group at http://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/d/optout.

Re: [PATCH 33/38] scsi: transport: add missing put_device call

2013-12-22 Thread James Bottomley 
On Thu, 2013-12-19 at 16:06 +0100, Levente Kurusa wrote:
 This is required so that we give up the last reference to the device.

This isn't true.

 Remove the kfree() as well, because the put_device() will result in
 iscsi_endpoint_release being called and hence it will be kfree'd.

There's no real point to this patch.  The use case where we own the
device absolutely up until the point we hand out references is well
established and there are a number of destroy paths running through SCSI
code which don't go via refcounting and this is one of them.  They're
almost universally on error legs on device bring up as this one is.

James


-- 
You received this message because you are subscribed to the Google Groups 
open-iscsi group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To post to this group, send email to open-iscsi@googlegroups.com.
Visit this group at http://groups.google.com/group/open-iscsi.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [PATCH] SCSI: amd_iommu dma_boundary overflow

2013-02-21 Thread James Bottomley 
This is a bit tricky, since AMD laid off the team who were maintaining
this, but I added to the cc' one of the original maintainers in the
hopes they can take a look.

James


On Tue, 2013-02-19 at 18:30 -0800, Eddie Wai wrote:
 Hello,
 
 For a 64-bit DMA capable PCIe storage HBA running under the 64-bit
 AMD-VI IOMMU environment, the amd_iommu code was observed to hit an
 overflow when it tries to page align the dma_parms-segment_boundary_mask.
 This overflow would eventually trigger the BUG_ON in the iommu-helper's
 iommu_is_span_boundary is_power_of_2 check.
 
 A little back tracking shows that since the device is 64-bit DMA capable,
 the pcidev-dma_mask was correctly set to DMA_BIT_MASK(64).  This dma_mask
 was then transferred to the SCSI host's dma_boundary param (by the iSCSI
 driver) and was eventually used to populate the q-limits.seg_boundary_mask
 (via blk_queue_segment_boundary) and the dma_parms-segment_boundary_mask
 (via dma_set_seg_boundary) during the scsi queue allocation.
 
 The code seems correct as it make sense to impose the same hardware
 segment boundary limit on both the blk queue and the DMA code.  It would
 be an easy alternative to simply prevent the shost-dma_boundary from
 being set to DMA_BIT_MASK(64), but it seems more correct to fix the
 amd_iommu code itself to detect and handle this max 64-bit mask condition.
 
 Please let me know your comments.
 
 Thanks,
 Eddie
 
 Signed-off-by: Eddie Wai eddie@broadcom.com
 ---
  drivers/iommu/amd_iommu.c |7 +--
  1 files changed, 5 insertions(+), 2 deletions(-)
 
 diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
 index d90a421..63185a1 100644
 --- a/drivers/iommu/amd_iommu.c
 +++ b/drivers/iommu/amd_iommu.c
 @@ -1526,11 +1526,14 @@ static unsigned long dma_ops_area_alloc(struct device 
 *dev,
   unsigned long boundary_size;
   unsigned long address = -1;
   unsigned long limit;
 + unsigned long mask;
  
   next_bit = PAGE_SHIFT;
  
 - boundary_size = ALIGN(dma_get_seg_boundary(dev) + 1,
 - PAGE_SIZE)  PAGE_SHIFT;
 + mask = dma_get_seg_boundary(dev);
 + boundary_size = mask + 1 ?
 + ALIGN(mask + 1, PAGE_SIZE)  PAGE_SHIFT :
 + 1UL  (BITS_PER_LONG - PAGE_SHIFT);
  
   for (;i  max_index; ++i) {
   unsigned long offset = dom-aperture[i]-offset  PAGE_SHIFT;

-- 
You received this message because you are subscribed to the Google Groups 
open-iscsi group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to open-iscsi+unsubscr...@googlegroups.com.
To post to this group, send email to open-iscsi@googlegroups.com.
Visit this group at http://groups.google.com/group/open-iscsi?hl=en.
For more options, visit https://groups.google.com/groups/opt_out.

Re: [PATCH 3/4 v2] BNX2I: Changed the nopout_wqe-lun memcpy to use sizeof instead

2011-06-30 Thread James Bottomley 
On Fri, 2011-06-24 at 15:20 -0500, Mike Christie wrote:
 On 06/23/2011 05:51 PM, Eddie Wai wrote:
  Modified the memcpy of nopout_wqe-lun to use sizeof(struct scsi_lun)
  instead of the hardcoded value 8 as noted by review comments.
  
  Signed-off-by: Eddie Wai eddie@broadcom.com
  ---
   drivers/scsi/bnx2i/bnx2i_hwi.c |2 +-
   1 files changed, 1 insertions(+), 1 deletions(-)
  
  diff --git a/drivers/scsi/bnx2i/bnx2i_hwi.c b/drivers/scsi/bnx2i/bnx2i_hwi.c
  index 2a1bb9f..ba2f96e 100644
  --- a/drivers/scsi/bnx2i/bnx2i_hwi.c
  +++ b/drivers/scsi/bnx2i/bnx2i_hwi.c
  @@ -551,7 +551,7 @@ int bnx2i_send_iscsi_nopout(struct bnx2i_conn 
  *bnx2i_conn,
   
  nopout_wqe-op_code = nopout_hdr-opcode;
  nopout_wqe-op_attr = ISCSI_FLAG_CMD_FINAL;
  -   memcpy(nopout_wqe-lun, nopout_hdr-lun, 8);
  +   memcpy(nopout_wqe-lun, nopout_hdr-lun, sizeof(struct scsi_lun));
   
  if (test_bit(BNX2I_NX2_DEV_57710, ep-hba-cnic_dev_type)) {
  u32 tmp = nopout_wqe-lun[0];
 
 This patch and Andy your patch [PATCH] iscsi: Use struct scsi_lun in
 iscsi structs instead of u8[8] are going to conflict with each other.
 
 And Eddie, I think you missed some other instances where 8 is hardcoded.

Agreed, I skipped this one for now ... please resubmit against scsi-misc
if more work is needed.

Thanks,

James


-- 
You received this message because you are subscribed to the Google Groups 
open-iscsi group.
To post to this group, send email to open-iscsi@googlegroups.com.
To unsubscribe from this group, send email to 
open-iscsi+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/open-iscsi?hl=en.

Re: [Security] [PATCH 00/20] world-writable files in sysfs and debugfs

2011-03-15 Thread James Bottomley 
On Mon, 2011-03-14 at 20:09 -0700, Greg KH wrote:
 On Mon, Mar 14, 2011 at 10:26:05PM -0400, James Bottomley wrote:
  On Sat, 2011-03-12 at 23:23 +0300, Vasiliy Kulikov wrote:
Vasiliy Kulikov (20):
 mach-ux500: mbox-db5500: world-writable sysfs fifo file
 leds: lp5521: world-writable sysfs engine* files
 leds: lp5523: world-writable engine* sysfs files
 misc: ep93xx_pwm: world-writable sysfs files
 rtc: rtc-ds1511: world-writable sysfs nvram file
 scsi: aic94xx: world-writable sysfs update_bios file
 scsi: iscsi: world-writable sysfs priv_sess file
   
   These are still not merged :(
  
  OK, so I've not been tracking where we are in the dizzying ride on
  security systems.  However, I thought we landed up in the privilege
  separation arena using capabilities.  That means that world writeable
  files aren't necessarily a problem as long as the correct capabilities
  checks are in place, right?
 
 There are no capability checks on sysfs files right now, so these all
 need to be fixed.

That statement is true but irrelevant, isn't it?  There can't be
capabilities within sysfs files because the system that does them has no
idea what the capabilities would be.  If there were capabilities checks,
they'd have to be in the implementing routines.

I think the questions are twofold:

 1. Did anyone actually check for capabilities before assuming world
writeable files were wrong?
 2. Even if there aren't any capabilities checks in the implementing
routines, should there be (are we going the separated
capabilities route vs the monolithic root route)?

James


-- 
You received this message because you are subscribed to the Google Groups 
open-iscsi group.
To post to this group, send email to open-iscsi@googlegroups.com.
To unsubscribe from this group, send email to 
open-iscsi+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/open-iscsi?hl=en.

Re: [PATCH 00/20] world-writable files in sysfs and debugfs

2011-03-15 Thread James Bottomley 
On Sat, 2011-03-12 at 23:23 +0300, Vasiliy Kulikov wrote:
  Vasiliy Kulikov (20):
   mach-ux500: mbox-db5500: world-writable sysfs fifo file
   leds: lp5521: world-writable sysfs engine* files
   leds: lp5523: world-writable engine* sysfs files
   misc: ep93xx_pwm: world-writable sysfs files
   rtc: rtc-ds1511: world-writable sysfs nvram file
   scsi: aic94xx: world-writable sysfs update_bios file
   scsi: iscsi: world-writable sysfs priv_sess file
 
 These are still not merged :(

OK, so I've not been tracking where we are in the dizzying ride on
security systems.  However, I thought we landed up in the privilege
separation arena using capabilities.  That means that world writeable
files aren't necessarily a problem as long as the correct capabilities
checks are in place, right?

James


-- 
You received this message because you are subscribed to the Google Groups 
open-iscsi group.
To post to this group, send email to open-iscsi@googlegroups.com.
To unsubscribe from this group, send email to 
open-iscsi+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/open-iscsi?hl=en.

Re: [Security] [PATCH 00/20] world-writable files in sysfs and debugfs

2011-03-15 Thread James Bottomley 
On Tue, 2011-03-15 at 07:18 -0700, Greg KH wrote:
 On Tue, Mar 15, 2011 at 07:50:28AM -0400, James Bottomley wrote:
  On Mon, 2011-03-14 at 20:09 -0700, Greg KH wrote:
   There are no capability checks on sysfs files right now, so these all
   need to be fixed.
  
  That statement is true but irrelevant, isn't it?  There can't be
  capabilities within sysfs files because the system that does them has no
  idea what the capabilities would be.  If there were capabilities checks,
  they'd have to be in the implementing routines.
 
 Ah, you are correct, sorry for the misunderstanding.
 
  I think the questions are twofold:
  
   1. Did anyone actually check for capabilities before assuming world
  writeable files were wrong?
 
 I do not think so as the majority (i.e. all the ones that I looked at)
 did no such checks.

OK, as long as someone checked, I'm happy.

   2. Even if there aren't any capabilities checks in the implementing
  routines, should there be (are we going the separated
  capabilities route vs the monolithic root route)?
 
 I think the general consensus is that we go the monolithic root route
 for sysfs files in that we do not allow them to be world writable.
 
 Do you have any exceptions that you know of that do these checks?

Heh, I didn't call our security vacillations a dizzying ride for
nothing.  I know the goal once was to try to run a distro without root
daemons (which is what required the capabilities stuff).  I'm actually
trying to avoid the issue ... I just want to make sure that people who
care aren't all moving in different directions.

James


-- 
You received this message because you are subscribed to the Google Groups 
open-iscsi group.
To post to this group, send email to open-iscsi@googlegroups.com.
To unsubscribe from this group, send email to 
open-iscsi+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/open-iscsi?hl=en.

Re: [Security] [PATCH 00/20] world-writable files in sysfs and debugfs

2011-03-15 Thread James Bottomley 
On Tue, 2011-03-15 at 19:08 +0300, Vasiliy Kulikov wrote:
 On Tue, Mar 15, 2011 at 07:50 -0400, James Bottomley wrote:
   1. Did anyone actually check for capabilities before assuming world
  writeable files were wrong?
 
 I didn't check all these files as I haven't got these hardware :-)

You don't need the hardware to check ... the question becomes is a
capabilities test sitting in the implementation or not.

   But
 as I can chmod a+w all sysfs files on my machine and they all become
 sensible to nonroot writes, I suppose there is nothing preventing
 nonroot users from writing to these buggy sysfs files.  As you can see,
 there are no capable() checks in these drivers in open() or write().
 
   2. Even if there aren't any capabilities checks in the implementing
  routines, should there be (are we going the separated
  capabilities route vs the monolithic root route)?
 
 IMO, In any case old good DAC security model must not be obsoleted just
 because someone thinks that MAC or anything else is more convenient for
 him.  If sysfs is implemented via filesystem then it must support POSIX
 permissions semantic.  MAC is very good in _some_ cases, but not instead
 of DAC.

Um, I'm not sure that's even an issue.  capabilities have CAP_ADMIN
which is precisely the same check as owner == root.  We use this a lot
because ioctls ignore the standard unix DAC model.

James



-- 
You received this message because you are subscribed to the Google Groups 
open-iscsi group.
To post to this group, send email to open-iscsi@googlegroups.com.
To unsubscribe from this group, send email to 
open-iscsi+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/open-iscsi?hl=en.

Re: [PATCH 1/5] BNX2I - Add 5771E device support to bnx2i driver

2009-12-10 Thread James Bottomley 
On Wed, 2009-12-09 at 20:45 -0600, Mike Christie wrote:
 Reviewed-by: Mike Christie micha...@cs.wisc.edu

But not by checkpatch:

ERROR: trailing whitespace
#23: FILE: drivers/scsi/bnx2i/bnx2i_init.c:90:
+^I^Iprintk(KERN_ALERT bnx2i: unknown device, 0x%x\n, $

total: 1 errors, 0 warnings, 13 lines checked

I've fixed it up.  And the several other whitespace problems in the rest
of the patch set.

James


--

You received this message because you are subscribed to the Google Groups 
open-iscsi group.
To post to this group, send email to open-is...@googlegroups.com.
To unsubscribe from this group, send email to 
open-iscsi+unsubscr...@googlegroups.com.
For more options, visit this group at 
http://groups.google.com/group/open-iscsi?hl=en.

Re: [PATCH 2.6.30-rc] cxgb3i -- fixed connection error when vlan is enabled

2009-06-27 Thread James Bottomley 

On Sat, 2009-06-27 at 12:52 -0500, Mike Christie wrote:
 On 06/26/2009 05:17 PM, k...@chelsio.com wrote:
  [PATCH 2.6.30-rc] cxgb3i -- fixed connection error when vlan is enabled
 
  From: Karen Xiek...@chelsio.com
 
  There is a bug when VLAN is configured on the cxgb3 interface, the iscsi
  conn. would be denied with message cxgb3i: NOT going through cxgbi device.
 
  This patch added code to get the real egress net_device when vlan is 
  configured.
 
  Signed-off-by: Karen Xiek...@chelsio.com
  ---
 
drivers/scsi/cxgb3i/cxgb3i_iscsi.c |4 
1 files changed, 4 insertions(+), 0 deletions(-)
 
 
  diff --git a/drivers/scsi/cxgb3i/cxgb3i_iscsi.c 
  b/drivers/scsi/cxgb3i/cxgb3i_iscsi.c
  index 04a4374..60013a4 100644
  --- a/drivers/scsi/cxgb3i/cxgb3i_iscsi.c
  +++ b/drivers/scsi/cxgb3i/cxgb3i_iscsi.c
  @@ -13,6 +13,7 @@
 
#includelinux/inet.h
#includelinux/crypto.h
  +#includelinux/if_vlan.h
#includenet/tcp.h
#includescsi/scsi_cmnd.h
#includescsi/scsi_device.h
  @@ -183,6 +184,9 @@ static struct cxgb3i_hba 
  *cxgb3i_hba_find_by_netdev(struct net_device *ndev)
  struct cxgb3i_adapter *snic;
  int i;
 
  +   if (ndev-priv_flags  IFF_802_1Q_VLAN)
  +   ndev = vlan_dev_real_dev(ndev);
  +
  read_lock(cxgb3i_snic_rwlock);
  list_for_each_entry(snic,cxgb3i_snic_list, list_head) {
  for (i = 0; i  snic-hba_cnt; i++) {
 
 
 It looks ok, but when touching network stuff you might want to cc the 
 netdev list in the future. Well, maybe for non-trivial stuff at least. 
 This might be fine.
 
 Reviewed-by: Mike Christie micha...@cs.wisc.edu

Thanks, but actually this patch gives a rejection here because of a
commit made by Herbert Xu which is already in mainline:

commit f00a3328bf9ecff46abd68a421693ba71cd16fc8
Author: Herbert Xu herb...@gondor.apana.org.au
Date:   Sat May 30 13:40:04 2009 +1000

[SCSI] cxgb3i: Include net/dst.h for struct dst_cache

It's simple enough to fix up, so I did it, but next time could you
(Karen) check your patches against the relevant trees to make sure
they're up to date.

James



--~--~-~--~~~---~--~~
You received this message because you are subscribed to the Google Groups 
open-iscsi group.
To post to this group, send email to open-iscsi@googlegroups.com
To unsubscribe from this group, send email to 
open-iscsi+unsubscr...@googlegroups.com
For more options, visit this group at http://groups.google.com/group/open-iscsi
-~--~~~~--~~--~--~---

Re: [PATCH 1/2 2.6.30-rc] cxgb3i -- add a refcnt to track ddp usage

2009-06-24 Thread James Bottomley 

On Sat, 2009-06-13 at 14:29 -0700, k...@chelsio.com wrote:
 [PATCH 1/2 2.6.30-rc] cxgb3i -- add a refcnt to track ddp usage
 
 From: Karen Xie k...@chelsio.com
 
 The iscsi ddp functionality could be used by multiple iscsi entities,
 add a refcnt to keep track of it, so we would not release it pre-maturely.

The code is fine as it stands but, I wonder, would you consider using a
kref for this?  The reasons are twofold:

 1. It's a well understood kernel paradigm that works ... this would
be helpful for the far distant time when chelsio no longer wants
to maintain this driver.
 2. It includes useful and appropriate warnings for common bugs in
refcounted code, such as use of a freed reference.

James



--~--~-~--~~~---~--~~
You received this message because you are subscribed to the Google Groups 
open-iscsi group.
To post to this group, send email to open-iscsi@googlegroups.com
To unsubscribe from this group, send email to 
open-iscsi+unsubscr...@googlegroups.com
For more options, visit this group at http://groups.google.com/group/open-iscsi
-~--~~~~--~~--~--~---

Re: [PATCH] iscsi_tcp - make padbuf non-static

2009-01-14 Thread James Bottomley 

On Thu, 2009-01-08 at 15:06 -0600, Mike Christie wrote:
 Karen Xie wrote:
  [PATCH] iscsi_tcp - make padbuf non-static
  
  From: Karen Xie k...@chelsio.com
  
  virt_to_page() call should not be used on kernel text and data addresses.
  virt_to_page() is used by sg_init_one(). So change padbuf to be allocated 
  within iscsi_segment.
  
  Signed-off-by: Karen Xie k...@chelsio.com
 
 
 Thanks for finding this.
 
 Acked-by: Mike Christie micha...@cs.wisc.edu

Could one of you regenerate this against current git head, please, it
seems to have suffered from the iscsi to libiscsi move.

James



--~--~-~--~~~---~--~~
You received this message because you are subscribed to the Google Groups 
open-iscsi group.
To post to this group, send email to open-iscsi@googlegroups.com
To unsubscribe from this group, send email to 
open-iscsi+unsubscr...@googlegroups.com
For more options, visit this group at http://groups.google.com/group/open-iscsi
-~--~~~~--~~--~--~---

Re: [PATCH 0/3 ver2] iscsi bidi varlen support

2008-02-18 Thread James Bottomley 

On Mon, 2008-02-18 at 17:08 +0200, Boaz Harrosh wrote:
 But ... James? is
 there any chance these can go into scsi-rc-fixes for the 2.6.25
 kernel? The reason they are so late was mainly because of a fallout
 in the merge process and a bug that was introduced because of that,
 but they were intended to go together with bidi into 2.6.25. Also
 as an important client code to the bidi-api that is introduced in
 2.6.25 kernel.

Well, I think you know the answer to that one under Linus' rules for non
merge window submission.  It's not a bug fix; we haven't even put it
into -mm for testing and it's a pretty invasive change.

James



--~--~-~--~~~---~--~~
You received this message because you are subscribed to the Google Groups 
open-iscsi group.
To post to this group, send email to open-iscsi@googlegroups.com
To unsubscribe from this group, send email to [EMAIL PROTECTED]
For more options, visit this group at http://groups.google.com/group/open-iscsi
-~--~~~~--~~--~--~---