On Tue, 2011-03-15 at 11:15 +0100, Christoph Hellwig wrote:
> On Mon, Mar 14, 2011 at 05:17:12PM -0700, Nicholas A. Bellinger wrote:
> > > > +#include <linux/version.h>
> > > > +#include <generated/utsrelease.h>
> > > > +#include <linux/utsname.h>
> > >
> > > You keep including these headers a lot, and I still don't understand why. Even
> > > if we need to expose data from it it should be done once in the core and not
> > > all over the code.
> > >
> >
> > OK, moved into a single iscsi_target_core.h include.
>
> That's not what I meant.
>
> - for linux/version.h:
>
> neither LINUX_VERSION_CODE nor KERNEL_VERSION is used anywhere in
> the target code, so it can't possibly required at all.
>
OK, dropped.
> - for linux/utsname.h and <generated/utsrelease.h>:
>
> please either remove the attributes printing this information
> into common code. or better off just remove it all all. You can
> get the kernel version and release from the utsname system call,
> there is absolutely no need to duplicate it in a different attribute in
> every target frontend.
Ok, sorry, I see what you mean wrt to UTS_RELEASE and dropping this
usage and generated/utsrelease.h include.
> Also printing it during initialization is completely pointless, too.
>
the 'vX.Y.Z on Linux/$ARCH' in the version string has been around for a
looong time, but I don't mind dropping it all together for the handful
of informational cases here.
> > > > +void core_put_tiqn_for_login(struct iscsi_tiqn *tiqn)
> > > > +{
> > > > + spin_lock(&tiqn->tiqn_state_lock);
> > > > + atomic_dec(&tiqn->tiqn_access_count);
> > > > + spin_unlock(&tiqn->tiqn_state_lock);
> > > > + return;
> > >
> > > no need for the return here. Also what's the point of making tiqn_access_count
> > > if you take a spinlock around all it's modifications?
> > >
> >
> > Removed the unnecessary return here.. I was behind paranoid here..
>
> Also please make tiqn_access_count a normal integer type, it is always
> protected by tiqn_state_lock.
>
Done.
> > Indeed.. Ok, I am going to go ahead an rename everything using core_ to
> > iscsi_
>
> Looks like none of the initiator code currently uses iscsi_, but I'd still
> feel better about iscsit_ to make sure we're not conflicting with other
> initiator side code.
>
Ok, will use this for the core_ prefixed bits..
> > > Can't you just use the core idr code for generating indices?
> > >
> >
> > Mmmm, not sure what you mean here..
>
> Take a look at include/linux/idr.h.
>
Thanks, looking at the code now.
> Note that the uses for np_index, tpg_np_index and conn_index can be
> removed entirely as they are unused.
>
Done
> > Ok, there are a quite a few struct semaphores that need to be converted
> > into a struct mutex or struct completion.. I will get all of these
> > converted over..
>
> or sometimes spinlocks. Or often removed at all, as they just implement weird
> semantics for the threads - no need to have startup/shutdown synchronization
> as the kthread semantics are synchronous, and for a wakeup after queueing
> up work a simple wake_up_process on the task_struct pointer is enough.
>
Ok, I will get this cleaned up for the NP login thread and TX conn
thread cases first and find+locate the issue mentioned above for the RX
case using kthread_stop()
> If you have question on how to avoid certain uses feel free to ask.
>
<nod>
> >
> > Ok, this is where I have previously run into some issues after doing a
> > kthread conversion for the RX/TX pairs using sock_recvmsg() some time a
> > while back. That said, I will go ahead will get the ulgiest pieces for
> > the NP thread converted first and then have another look where the RX
> > path code was (I think) having an issue to successfully perform iSCSI
> > Logout Request -> Response processing..
>
> Note that you might get away with less copies using the sk_data_ready,
> sk_state_change callbacks and tcp_read_sock() and totally dropping the
> traditional recvmsg path. As this remove the blocking behaviour it
> will as a side effect also remove any issues with thread startup/stop.
>
> Take a look at drivers/scsi/iscsi_tcp.c for an implementation.
>
<nod>, I am well aware of ->sk_data_ready() for directly accessing
struct sk_buff memory for the RX side case, and currently consider this
a future optimiziation item.
> > > > +#define CONN(cmd) ((struct iscsi_conn *)(cmd)->conn)
> > > > +#define CONN_OPS(conn) ((struct iscsi_conn_ops *)(conn)->conn_ops)
> > >
> > > There really shouldn't be any need for these macros.
> > >
> > >
> > > > +#define SESS(conn) ((struct iscsi_session *)(conn)->sess)
> > > > +#define SESS_OPS(sess) ((struct iscsi_sess_ops *)(sess)->sess_ops)
> > > > +#define SESS_OPS_C(conn) ((struct iscsi_sess_ops *)(conn)->sess->sess_ops)
> > > > +#define SESS_NODE_ACL(sess) ((struct se_node_acl *)(sess)->se_sess->se_node_acl)
> > >
> > > Same here.
> > >
> >
> > Ok, I will drop the unnecessary casts here, and I will look at thinning
> > -> removing these out these handful of macros.
>
> It's not just the casts - macros just for dereferencing a field just obsfucate
> the code.
>
Yep, will get them cleaned up as the larger items are addressed.
Thanks Christoph!
--nab
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
