[PATCH 0/6] Last batch of lnet selftest cleanup

[PATCH 0/6] Last batch of lnet selftest cleanup

[James Simmons]

Some fixes from two earlier patches got dropped so we add them to
this batch. Last of the style cleanups for LNet selftest. Redid
the comment style patch to format the comments correctly.

Dmitry Eremin (1):
  staging: lustre: add missing buffer overflow fix for console.c

James Nunez (1):
  staging: lustre: Correct missing newline for CERROR call in sfw_handle_server_rpc

James Simmons (4):
  staging: lustre: handle error returned from wait_event_timeout seltest timer
  staging: lustre: remove excess blank lines in lnet selftest code
  staging: lustre: realign some code in lnet selftest so its readable
  staging: lustre: cleanup comment style for lnet selftest

 drivers/staging/lustre/lnet/selftest/brw_test.c  |    4 +-
 drivers/staging/lustre/lnet/selftest/conctl.c    |   50 +++++++++++-----------
 drivers/staging/lustre/lnet/selftest/conrpc.c    |   19 ++++----
 drivers/staging/lustre/lnet/selftest/console.c   |   11 ++---
 drivers/staging/lustre/lnet/selftest/framework.c |   14 +++---
 drivers/staging/lustre/lnet/selftest/ping_test.c |    6 +-
 drivers/staging/lustre/lnet/selftest/rpc.c       |    8 ++--
 drivers/staging/lustre/lnet/selftest/timer.c     |   12 +++--
 8 files changed, 61 insertions(+), 63 deletions(-)

[PATCH 1/6] staging: lustre: Correct missing newline for CERROR call in sfw_handle_server_rpc

[James Simmons]

From: James Nunez <james.a.nunez@intel.com>

This is one of the fixes broken out of patch 10000 that was
missed in the merger. With this fix the CERROR called in
sfw_handle_server_rpc will print out correctly.

Signed-off-by: James Nunez <james.a.nunez@intel.com>
Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-4871
Reviewed-on: http://review.whamcloud.com/10000
Reviewed-by: Andreas Dilger <andreas.dilger@intel.com>
Reviewed-by: John L. Hammond <john.hammond@intel.com>
Reviewed-by: Cliff White <cliff.white@intel.com>
---
 drivers/staging/lustre/lnet/selftest/framework.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/staging/lustre/lnet/selftest/framework.c b/drivers/staging/lustre/lnet/selftest/framework.c
index 926c397..0f32f0b 100644
--- a/drivers/staging/lustre/lnet/selftest/framework.c
+++ b/drivers/staging/lustre/lnet/selftest/framework.c
@@ -1244,7 +1244,7 @@ sfw_handle_server_rpc(struct srpc_server_rpc *rpc)
 
 	/* Remove timer to avoid racing with it or expiring active session */
 	if (sfw_del_session_timer()) {
-		CERROR("Dropping RPC (%s) from %s: racing with expiry timer.",
+		CERROR("dropping RPC %s from %s: racing with expiry timer\n",
 		       sv->sv_name, libcfs_id2str(rpc->srpc_peer));
 		spin_unlock(&sfw_data.fw_lock);
 		return -EAGAIN;
-- 
1.7.1

[PATCH 2/6] staging: lustre: add missing buffer overflow fix for console.c

[James Simmons]

From: Dmitry Eremin <dmitry.eremin@intel.com>

Patch 9389 change a strncpy call into a strlcpy call. This was
missed in the merger into the upstream client.

Signed-off-by: Dmitry Eremin <dmitry.eremin@intel.com>
Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-4629
Reviewed-on: http://review.whamcloud.com/9389
Reviewed-by: Andreas Dilger <andreas.dilger@intel.com>
Reviewed-by: Oleg Drokin <oleg.drokin@intel.com>
---
 drivers/staging/lustre/lnet/selftest/console.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/staging/lustre/lnet/selftest/console.c b/drivers/staging/lustre/lnet/selftest/console.c
index 1a923ea..c009ad3 100644
--- a/drivers/staging/lustre/lnet/selftest/console.c
+++ b/drivers/staging/lustre/lnet/selftest/console.c
@@ -1749,7 +1749,7 @@ lstcon_session_new(char *name, int key, unsigned feats,
 
 	if (strlen(name) > sizeof(console_session.ses_name) - 1)
 		return -E2BIG;
-	strncpy(console_session.ses_name, name,
+	strlcpy(console_session.ses_name, name,
 		sizeof(console_session.ses_name));
 
 	rc = lstcon_batch_add(LST_DEFAULT_BATCH);
-- 
1.7.1

[PATCH 3/6] staging: lustre: handle error returned from wait_event_timeout seltest timer

[James Simmons]

The function wait_event_timeout can fail and return an error. Handle
this case in stt_timer_main().

Signed-off-by: James Simmons <jsimmons@infradead.org>
---
 drivers/staging/lustre/lnet/selftest/timer.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/lustre/lnet/selftest/timer.c b/drivers/staging/lustre/lnet/selftest/timer.c
index 8be5252..ef8a8a7 100644
--- a/drivers/staging/lustre/lnet/selftest/timer.c
+++ b/drivers/staging/lustre/lnet/selftest/timer.c
@@ -170,20 +170,22 @@ stt_check_timers(unsigned long *last)
 static int
 stt_timer_main(void *arg)
 {
+	int rc = 0;
+
 	cfs_block_allsigs();
 
 	while (!stt_data.stt_shuttingdown) {
 		stt_check_timers(&stt_data.stt_prev_slot);
 
-		wait_event_timeout(stt_data.stt_waitq,
-				   stt_data.stt_shuttingdown,
-				   cfs_time_seconds(STTIMER_SLOTTIME));
+		rc = wait_event_timeout(stt_data.stt_waitq,
+					stt_data.stt_shuttingdown,
+					cfs_time_seconds(STTIMER_SLOTTIME));
 	}
 
 	spin_lock(&stt_data.stt_lock);
 	stt_data.stt_nthreads--;
 	spin_unlock(&stt_data.stt_lock);
-	return 0;
+	return rc;
 }
 
 static int
-- 
1.7.1

[PATCH 4/6] staging: lustre: remove excess blank lines in lnet selftest code

[James Simmons]

Remove extra blank lines missed by checkpatch.

Signed-off-by: James Simmons <jsimmons@infradead.org>
---
 drivers/staging/lustre/lnet/selftest/brw_test.c |    2 --
 drivers/staging/lustre/lnet/selftest/conrpc.c   |    2 --
 drivers/staging/lustre/lnet/selftest/console.c  |    2 --
 3 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/lustre/lnet/selftest/brw_test.c b/drivers/staging/lustre/lnet/selftest/brw_test.c
index eebc924..69812fc 100644
--- a/drivers/staging/lustre/lnet/selftest/brw_test.c
+++ b/drivers/staging/lustre/lnet/selftest/brw_test.c
@@ -91,7 +91,6 @@ brw_client_init(sfw_test_instance_t *tsi)
 		 * but we have to keep it for compatibility
 		 */
 		len = npg * PAGE_CACHE_SIZE;
-
 	} else {
 		test_bulk_req_v1_t *breq = &tsi->tsi_u.bulk_v1;
 
@@ -279,7 +278,6 @@ brw_client_prep_rpc(sfw_test_unit_t *tsu,
 		flags = breq->blk_flags;
 		npg = breq->blk_npg;
 		len = npg * PAGE_CACHE_SIZE;
-
 	} else {
 		test_bulk_req_v1_t *breq = &tsi->tsi_u.bulk_v1;
 
diff --git a/drivers/staging/lustre/lnet/selftest/conrpc.c b/drivers/staging/lustre/lnet/selftest/conrpc.c
index bcd7888..d11869a 100644
--- a/drivers/staging/lustre/lnet/selftest/conrpc.c
+++ b/drivers/staging/lustre/lnet/selftest/conrpc.c
@@ -531,7 +531,6 @@ lstcon_rpc_trans_interpreter(lstcon_rpc_trans_t *trans,
 			continue;
 
 		error = readent(trans->tas_opc, msg, ent);
-
 		if (error)
 			return error;
 	}
@@ -841,7 +840,6 @@ lstcon_testrpc_prep(lstcon_node_t *nd, int transop, unsigned feats,
 
 		trq->tsr_ndest = 0;
 		trq->tsr_loop = nmax * test->tes_dist * test->tes_concur;
-
 	} else {
 		bulk = &(*crpc)->crp_rpc->crpc_bulk;
 
diff --git a/drivers/staging/lustre/lnet/selftest/console.c b/drivers/staging/lustre/lnet/selftest/console.c
index c009ad3..17d0d13 100644
--- a/drivers/staging/lustre/lnet/selftest/console.c
+++ b/drivers/staging/lustre/lnet/selftest/console.c
@@ -977,7 +977,6 @@ lstcon_batch_info(char *name, lstcon_test_batch_ent_t __user *ent_up,
 	if (!test) {
 		entp->u.tbe_batch.bae_ntest = bat->bat_ntest;
 		entp->u.tbe_batch.bae_state = bat->bat_state;
-
 	} else {
 		entp->u.tbe_test.tse_type = test->tes_type;
 		entp->u.tbe_test.tse_loop = test->tes_loop;
@@ -1423,7 +1422,6 @@ lstcon_test_batch_query(char *name, int testidx, int client,
 		translist = &batch->bat_trans_list;
 		ndlist = &batch->bat_cli_list;
 		hdr = &batch->bat_hdr;
-
 	} else {
 		/* query specified test only */
 		rc = lstcon_test_find(batch, testidx, &test);
-- 
1.7.1

[PATCH 5/6] staging: lustre: realign some code in lnet selftest so its readable

[James Simmons]

Two places to align the code so it is easier to read.

Signed-off-by: James Simmons <jsimmons@infradead.org>
---
 drivers/staging/lustre/lnet/selftest/console.c   |    2 +-
 drivers/staging/lustre/lnet/selftest/ping_test.c |    4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/lustre/lnet/selftest/console.c b/drivers/staging/lustre/lnet/selftest/console.c
index 17d0d13..6ec8952 100644
--- a/drivers/staging/lustre/lnet/selftest/console.c
+++ b/drivers/staging/lustre/lnet/selftest/console.c
@@ -733,7 +733,7 @@ lstcon_group_list(int index, int len, char __user *name_up)
 	list_for_each_entry(grp, &console_session.ses_grp_list, grp_link) {
 		if (!index--) {
 			return copy_to_user(name_up, grp->grp_name, len) ?
-			       -EFAULT : 0;
+					    -EFAULT : 0;
 		}
 	}
 
diff --git a/drivers/staging/lustre/lnet/selftest/ping_test.c b/drivers/staging/lustre/lnet/selftest/ping_test.c
index 81a4504..438fca2 100644
--- a/drivers/staging/lustre/lnet/selftest/ping_test.c
+++ b/drivers/staging/lustre/lnet/selftest/ping_test.c
@@ -86,8 +86,8 @@ ping_client_fini(sfw_test_instance_t *tsi)
 }
 
 static int
-ping_client_prep_rpc(sfw_test_unit_t *tsu,
-		     lnet_process_id_t dest, srpc_client_rpc_t **rpc)
+ping_client_prep_rpc(sfw_test_unit_t *tsu, lnet_process_id_t dest,
+		     srpc_client_rpc_t **rpc)
 {
 	srpc_ping_reqst_t *req;
 	sfw_test_instance_t *tsi = tsu->tsu_instance;
-- 
1.7.1

[PATCH 6/6] staging: lustre: cleanup comment style for lnet selftest

[James Simmons]

Apply a consistent style for comments in the lnet selftest
code. Realign some of the comments to make it easier to read.
This also fixes a few checkpatch issues as well.

Signed-off-by: James Simmons <jsimmons@infradead.org>
---
 drivers/staging/lustre/lnet/selftest/brw_test.c  |    2 +-
 drivers/staging/lustre/lnet/selftest/conctl.c    |   50 +++++++++++-----------
 drivers/staging/lustre/lnet/selftest/conrpc.c    |   17 ++++----
 drivers/staging/lustre/lnet/selftest/console.c   |    5 +-
 drivers/staging/lustre/lnet/selftest/framework.c |   12 +++---
 drivers/staging/lustre/lnet/selftest/ping_test.c |    2 +-
 drivers/staging/lustre/lnet/selftest/rpc.c       |    8 ++--
 drivers/staging/lustre/lnet/selftest/timer.c     |    2 +-
 8 files changed, 50 insertions(+), 48 deletions(-)

diff --git a/drivers/staging/lustre/lnet/selftest/brw_test.c b/drivers/staging/lustre/lnet/selftest/brw_test.c
index 69812fc..b33c356 100644
--- a/drivers/staging/lustre/lnet/selftest/brw_test.c
+++ b/drivers/staging/lustre/lnet/selftest/brw_test.c
@@ -327,7 +327,7 @@ brw_client_done_rpc(sfw_test_unit_t *tsu, srpc_client_rpc_t *rpc)
 	if (rpc->crpc_status) {
 		CERROR("BRW RPC to %s failed with %d\n",
 		       libcfs_id2str(rpc->crpc_dest), rpc->crpc_status);
-		if (!tsi->tsi_stopping) /* rpc could have been aborted */
+		if (!tsi->tsi_stopping)	/* rpc could have been aborted */
 			atomic_inc(&sn->sn_brw_errors);
 		return;
 	}
diff --git a/drivers/staging/lustre/lnet/selftest/conctl.c b/drivers/staging/lustre/lnet/selftest/conctl.c
index 5c7cb72..6e2a81d 100644
--- a/drivers/staging/lustre/lnet/selftest/conctl.c
+++ b/drivers/staging/lustre/lnet/selftest/conctl.c
@@ -51,9 +51,9 @@ lst_session_new_ioctl(lstio_session_new_args_t *args)
 	char *name;
 	int rc;
 
-	if (!args->lstio_ses_idp || /* address for output sid */
-	    !args->lstio_ses_key ||    /* no key is specified */
-	    !args->lstio_ses_namep || /* session name */
+	if (!args->lstio_ses_idp ||	/* address for output sid */
+	    !args->lstio_ses_key ||	/* no key is specified */
+	    !args->lstio_ses_namep ||	/* session name */
 	    args->lstio_ses_nmlen <= 0 ||
 	    args->lstio_ses_nmlen > LST_NAME_SIZE)
 		return -EINVAL;
@@ -95,11 +95,11 @@ lst_session_info_ioctl(lstio_session_info_args_t *args)
 {
 	/* no checking of key */
 
-	if (!args->lstio_ses_idp || /* address for output sid */
-	    !args->lstio_ses_keyp || /* address for output key */
-	    !args->lstio_ses_featp || /* address for output features */
-	    !args->lstio_ses_ndinfo || /* address for output ndinfo */
-	    !args->lstio_ses_namep || /* address for output name */
+	if (!args->lstio_ses_idp ||	/* address for output sid */
+	    !args->lstio_ses_keyp ||	/* address for output key */
+	    !args->lstio_ses_featp ||	/* address for output features */
+	    !args->lstio_ses_ndinfo ||	/* address for output ndinfo */
+	    !args->lstio_ses_namep ||	/* address for output name */
 	    args->lstio_ses_nmlen <= 0 ||
 	    args->lstio_ses_nmlen > LST_NAME_SIZE)
 		return -EINVAL;
@@ -125,7 +125,7 @@ lst_debug_ioctl(lstio_debug_args_t *args)
 	if (!args->lstio_dbg_resultp)
 		return -EINVAL;
 
-	if (args->lstio_dbg_namep && /* name of batch/group */
+	if (args->lstio_dbg_namep &&	/* name of batch/group */
 	    (args->lstio_dbg_nmlen <= 0 ||
 	     args->lstio_dbg_nmlen > LST_NAME_SIZE))
 		return -EINVAL;
@@ -326,7 +326,7 @@ lst_nodes_add_ioctl(lstio_group_nodes_args_t *args)
 	if (args->lstio_grp_key != console_session.ses_key)
 		return -EACCES;
 
-	if (!args->lstio_grp_idsp || /* array of ids */
+	if (!args->lstio_grp_idsp ||	/* array of ids */
 	    args->lstio_grp_count <= 0 ||
 	    !args->lstio_grp_resultp ||
 	    !args->lstio_grp_featp ||
@@ -394,13 +394,13 @@ lst_group_info_ioctl(lstio_group_info_args_t *args)
 	    args->lstio_grp_nmlen > LST_NAME_SIZE)
 		return -EINVAL;
 
-	if (!args->lstio_grp_entp &&  /* output: group entry */
-	    !args->lstio_grp_dentsp)  /* output: node entry */
+	if (!args->lstio_grp_entp &&	/* output: group entry */
+	    !args->lstio_grp_dentsp)	/* output: node entry */
 		return -EINVAL;
 
-	if (args->lstio_grp_dentsp) { /* have node entry */
-		if (!args->lstio_grp_idxp || /* node index */
-		    !args->lstio_grp_ndentp) /* # of node entry */
+	if (args->lstio_grp_dentsp) {		/* have node entry */
+		if (!args->lstio_grp_idxp ||	/* node index */
+		    !args->lstio_grp_ndentp)	/* # of node entry */
 			return -EINVAL;
 
 		if (copy_from_user(&ndent, args->lstio_grp_ndentp,
@@ -612,18 +612,18 @@ lst_batch_info_ioctl(lstio_batch_info_args_t *args)
 	if (args->lstio_bat_key != console_session.ses_key)
 		return -EACCES;
 
-	if (!args->lstio_bat_namep || /* batch name */
+	if (!args->lstio_bat_namep ||	/* batch name */
 	    args->lstio_bat_nmlen <= 0 ||
 	    args->lstio_bat_nmlen > LST_NAME_SIZE)
 		return -EINVAL;
 
-	if (!args->lstio_bat_entp && /* output: batch entry */
-	    !args->lstio_bat_dentsp) /* output: node entry */
+	if (!args->lstio_bat_entp &&	/* output: batch entry */
+	    !args->lstio_bat_dentsp)	/* output: node entry */
 		return -EINVAL;
 
-	if (args->lstio_bat_dentsp) { /* have node entry */
-		if (!args->lstio_bat_idxp || /* node index */
-		    !args->lstio_bat_ndentp) /* # of node entry */
+	if (args->lstio_bat_dentsp) {		/* have node entry */
+		if (!args->lstio_bat_idxp ||	/* node index */
+		    !args->lstio_bat_ndentp)	/* # of node entry */
 			return -EINVAL;
 
 		if (copy_from_user(&index, args->lstio_bat_idxp,
@@ -722,18 +722,18 @@ static int lst_test_add_ioctl(lstio_test_args_t *args)
 
 	if (!args->lstio_tes_resultp ||
 	    !args->lstio_tes_retp ||
-	    !args->lstio_tes_bat_name || /* no specified batch */
+	    !args->lstio_tes_bat_name ||	/* no specified batch */
 	    args->lstio_tes_bat_nmlen <= 0 ||
 	    args->lstio_tes_bat_nmlen > LST_NAME_SIZE ||
-	    !args->lstio_tes_sgrp_name || /* no source group */
+	    !args->lstio_tes_sgrp_name ||	/* no source group */
 	    args->lstio_tes_sgrp_nmlen <= 0 ||
 	    args->lstio_tes_sgrp_nmlen > LST_NAME_SIZE ||
-	    !args->lstio_tes_dgrp_name || /* no target group */
+	    !args->lstio_tes_dgrp_name ||	/* no target group */
 	    args->lstio_tes_dgrp_nmlen <= 0 ||
 	    args->lstio_tes_dgrp_nmlen > LST_NAME_SIZE)
 		return -EINVAL;
 
-	if (!args->lstio_tes_loop || /* negative is infinite */
+	if (!args->lstio_tes_loop ||		/* negative is infinite */
 	    args->lstio_tes_concur <= 0 ||
 	    args->lstio_tes_dist <= 0 ||
 	    args->lstio_tes_span <= 0)
diff --git a/drivers/staging/lustre/lnet/selftest/conrpc.c b/drivers/staging/lustre/lnet/selftest/conrpc.c
index d11869a..3908c10 100644
--- a/drivers/staging/lustre/lnet/selftest/conrpc.c
+++ b/drivers/staging/lustre/lnet/selftest/conrpc.c
@@ -296,8 +296,8 @@ lstcon_rpc_trans_abort(lstcon_rpc_trans_t *trans, int error)
 
 		spin_lock(&rpc->crpc_lock);
 
-		if (!crpc->crp_posted || /* not posted */
-		    crpc->crp_stamp) { /* rpc done or aborted already */
+		if (!crpc->crp_posted ||	/* not posted */
+		    crpc->crp_stamp) {		/* rpc done or aborted already */
 			if (!crpc->crp_stamp) {
 				crpc->crp_stamp = cfs_time_current();
 				crpc->crp_status = -EINTR;
@@ -562,10 +562,10 @@ lstcon_rpc_trans_destroy(lstcon_rpc_trans_t *trans)
 		}
 
 		/*
-		 * rpcs can be still not callbacked (even LNetMDUnlink is called)
-		 * because huge timeout for inaccessible network, don't make
-		 * user wait for them, just abandon them, they will be recycled
-		 * in callback
+		 * rpcs can be still not callbacked (even LNetMDUnlink is
+		 * called) because huge timeout for inaccessible network,
+		 * don't make user wait for them, just abandon them, they
+		 * will be recycled in callback
 		 */
 		LASSERT(crpc->crp_status);
 
@@ -938,7 +938,7 @@ lstcon_sesnew_stat_reply(lstcon_rpc_trans_t *trans,
 
 	if (!trans->tas_feats_updated) {
 		spin_lock(&console_session.ses_rpc_lock);
-		if (!trans->tas_feats_updated) { /* recheck with lock */
+		if (!trans->tas_feats_updated) {	/* recheck with lock */
 			trans->tas_feats_updated = 1;
 			trans->tas_features = reply->msg_ses_feats;
 		}
@@ -1178,7 +1178,8 @@ lstcon_rpc_pinger(void *arg)
 	int count = 0;
 	int rc;
 
-	/* RPC pinger is a special case of transaction,
+	/*
+	 * RPC pinger is a special case of transaction,
 	 * it's called by timer at 8 seconds interval.
 	 */
 	mutex_lock(&console_session.ses_mutex);
diff --git a/drivers/staging/lustre/lnet/selftest/console.c b/drivers/staging/lustre/lnet/selftest/console.c
index 6ec8952..dcfc83d 100644
--- a/drivers/staging/lustre/lnet/selftest/console.c
+++ b/drivers/staging/lustre/lnet/selftest/console.c
@@ -277,7 +277,7 @@ lstcon_group_find(const char *name, lstcon_group_t **grpp)
 		if (strncmp(grp->grp_name, name, LST_NAME_SIZE))
 			continue;
 
-		lstcon_group_addref(grp);  /* +1 ref for caller */
+		lstcon_group_addref(grp); /* +1 ref for caller */
 		*grpp = grp;
 		return 0;
 	}
@@ -1446,7 +1446,8 @@ lstcon_test_batch_query(char *name, int testidx, int client,
 
 	lstcon_rpc_trans_postwait(trans, timeout);
 
-	if (!testidx && /* query a batch, not a test */
+	/* query a batch, not a test */
+	if (!testidx &&
 	    !lstcon_rpc_stat_failure(lstcon_trans_stat(), 0) &&
 	    !lstcon_tsbqry_stat_run(lstcon_trans_stat(), 0)) {
 		/* all RPCs finished, and no active test */
diff --git a/drivers/staging/lustre/lnet/selftest/framework.c b/drivers/staging/lustre/lnet/selftest/framework.c
index 0f32f0b..aa646a7 100644
--- a/drivers/staging/lustre/lnet/selftest/framework.c
+++ b/drivers/staging/lustre/lnet/selftest/framework.c
@@ -226,7 +226,7 @@ __must_hold(&sfw_data.fw_lock)
 	}
 
 	if (nactive)
-		return;   /* wait for active batches to stop */
+		return;	/* wait for active batches to stop */
 
 	list_del_init(&sn->sn_list);
 	spin_unlock(&sfw_data.fw_lock);
@@ -693,7 +693,7 @@ sfw_unpack_addtest_req(srpc_msg_t *msg)
 	LASSERT(req->tsr_is_client);
 
 	if (msg->msg_magic == SRPC_MSG_MAGIC)
-		return; /* no flipping needed */
+		return;	/* no flipping needed */
 
 	LASSERT(msg->msg_magic == __swab32(SRPC_MSG_MAGIC));
 
@@ -789,7 +789,7 @@ sfw_add_test_instance(sfw_batch_t *tsb, struct srpc_server_rpc *rpc)
 		int j;
 
 		dests = page_address(bk->bk_iovs[i / SFW_ID_PER_PAGE].kiov_page);
-		LASSERT(dests);  /* my pages are within KVM always */
+		LASSERT(dests);		/* my pages are within KVM always */
 		id = dests[i % SFW_ID_PER_PAGE];
 		if (msg->msg_magic != SRPC_MSG_MAGIC)
 			sfw_unpack_id(id);
@@ -844,8 +844,8 @@ sfw_test_unit_done(sfw_test_unit_t *tsu)
 
 	spin_lock(&sfw_data.fw_lock);
 
-	if (!atomic_dec_and_test(&tsb->bat_nactive) ||/* tsb still active */
-	    sn == sfw_data.fw_session) {		  /* sn also active */
+	if (!atomic_dec_and_test(&tsb->bat_nactive) ||	/* tsb still active */
+	    sn == sfw_data.fw_session) {		/* sn also active */
 		spin_unlock(&sfw_data.fw_lock);
 		return;
 	}
@@ -1273,7 +1273,7 @@ sfw_handle_server_rpc(struct srpc_server_rpc *rpc)
 		}
 
 	} else if (request->msg_ses_feats & ~LST_FEATS_MASK) {
-		/**
+		/*
 		 * NB: at this point, old version will ignore features and
 		 * create new session anyway, so console should be able
 		 * to handle this
diff --git a/drivers/staging/lustre/lnet/selftest/ping_test.c b/drivers/staging/lustre/lnet/selftest/ping_test.c
index 438fca2..c7c50be 100644
--- a/drivers/staging/lustre/lnet/selftest/ping_test.c
+++ b/drivers/staging/lustre/lnet/selftest/ping_test.c
@@ -129,7 +129,7 @@ ping_client_done_rpc(sfw_test_unit_t *tsu, srpc_client_rpc_t *rpc)
 	LASSERT(sn);
 
 	if (rpc->crpc_status) {
-		if (!tsi->tsi_stopping) /* rpc could have been aborted */
+		if (!tsi->tsi_stopping)	/* rpc could have been aborted */
 			atomic_inc(&sn->sn_ping_errors);
 		CERROR("Unable to ping %s (%d): %d\n",
 		       libcfs_id2str(rpc->crpc_dest),
diff --git a/drivers/staging/lustre/lnet/selftest/rpc.c b/drivers/staging/lustre/lnet/selftest/rpc.c
index 69be7d6..ab8ba37 100644
--- a/drivers/staging/lustre/lnet/selftest/rpc.c
+++ b/drivers/staging/lustre/lnet/selftest/rpc.c
@@ -1332,8 +1332,8 @@ srpc_abort_rpc(srpc_client_rpc_t *rpc, int why)
 {
 	LASSERT(why);
 
-	if (rpc->crpc_aborted || /* already aborted */
-	    rpc->crpc_closed)	 /* callback imminent */
+	if (rpc->crpc_aborted ||	/* already aborted */
+	    rpc->crpc_closed)		/* callback imminent */
 		return;
 
 	CDEBUG(D_NET, "Aborting RPC: service %d, peer %s, state %s, why %d\n",
@@ -1401,7 +1401,7 @@ srpc_send_reply(struct srpc_server_rpc *rpc)
 				   rpc->srpc_peer, rpc->srpc_self,
 				   &rpc->srpc_replymdh, ev);
 	if (rc)
-		ev->ev_fired = 1;  /* no more event expected */
+		ev->ev_fired = 1; /* no more event expected */
 	return rc;
 }
 
@@ -1509,7 +1509,7 @@ srpc_lnet_ev_handler(lnet_event_t *ev)
 			scd->scd_buf_err = 0;
 		}
 
-		if (!scd->scd_buf_err && /* adding buffer is enabled */
+		if (!scd->scd_buf_err &&	/* adding buffer is enabled */
 		    !scd->scd_buf_adjust &&
 		    scd->scd_buf_nposted < scd->scd_buf_low) {
 			scd->scd_buf_adjust = max(scd->scd_buf_total / 2,
diff --git a/drivers/staging/lustre/lnet/selftest/timer.c b/drivers/staging/lustre/lnet/selftest/timer.c
index ef8a8a7..b6c4aae 100644
--- a/drivers/staging/lustre/lnet/selftest/timer.c
+++ b/drivers/staging/lustre/lnet/selftest/timer.c
@@ -49,7 +49,7 @@
  * sorted by increasing expiry time. The number of slots is 2**7 (128),
  * to cover a time period of 1024 seconds into the future before wrapping.
  */
-#define STTIMER_MINPOLL        3   /* log2 min poll interval (8 s) */
+#define STTIMER_MINPOLL        3	/* log2 min poll interval (8 s) */
 #define STTIMER_SLOTTIME       (1 << STTIMER_MINPOLL)
 #define STTIMER_SLOTTIMEMASK   (~(STTIMER_SLOTTIME - 1))
 #define STTIMER_NSLOTS	       (1 << 7)
-- 
1.7.1

Re: [PATCH 1/6] staging: lustre: Correct missing newline for CERROR call in sfw_handle_server_rpc

[Joe Perches]

On Sat, 2016-03-12 at 13:00 -0500, James Simmons wrote:
> From: James Nunez <james.a.nunez@intel.com>
> 
> This is one of the fixes broken out of patch 10000 that was
> missed in the merger. With this fix the CERROR called in
> sfw_handle_server_rpc will print out correctly.

Speaking of CERROR and logging, it it really useful
for each CERROR use to have 2 static structs?

In CERROR -> CDEBUG_LIMIT there is a:
	static struct cfs_debug_limit_state cdls;
	(12 or 16 bytes depending on 32/64 bit arch)

and in CDEBUG_LIMIT -> _CDEBUG
	static struct libcfs_debug_msg_data msgdata;
	(24 or 36 bytes depending on 32/64 bit arch)

That seems a largish bit of data and code to initialize
these structs for over a thousand call sites.

Wouldn't a single static suffice?

Re: [PATCH 1/6] staging: lustre: Correct missing newline for CERROR call in sfw_handle_server_rpc

[Drokin, Oleg]

On Mar 12, 2016, at 1:23 PM, Joe Perches wrote:

> On Sat, 2016-03-12 at 13:00 -0500, James Simmons wrote:
>> From: James Nunez <james.a.nunez@intel.com>
>> 
>> This is one of the fixes broken out of patch 10000 that was
>> missed in the merger. With this fix the CERROR called in
>> sfw_handle_server_rpc will print out correctly.
> 
> Speaking of CERROR and logging, it it really useful
> for each CERROR use to have 2 static structs?
> 
> In CERROR -> CDEBUG_LIMIT there is a:
> 	static struct cfs_debug_limit_state cdls;
> 	(12 or 16 bytes depending on 32/64 bit arch)
> 
> and in CDEBUG_LIMIT -> _CDEBUG
> 	static struct libcfs_debug_msg_data msgdata;
> 	(24 or 36 bytes depending on 32/64 bit arch)
> 
> That seems a largish bit of data and code to initialize
> these structs for over a thousand call sites.
> 
> Wouldn't a single static suffice?

Single static would not work because the code is parallel so it'll
stomp over each other. or do you mean to have a common
structure for every callsite (but instantiated separately)?

This used to be a local structure in the past, but that
caused considerable stack growth for some functions, that added
up along the call chain, and that was the solution we came up with
that did help.

Re: [PATCH 1/6] staging: lustre: Correct missing newline for CERROR call in sfw_handle_server_rpc

[Joe Perches]

On Sat, 2016-03-12 at 18:32 +0000, Drokin, Oleg wrote:
> On Mar 12, 2016, at 1:23 PM, Joe Perches wrote:
> > On Sat, 2016-03-12 at 13:00 -0500, James Simmons wrote:
> > > From: James Nunez <james.a.nunez@intel.com>
> > > 
> > > This is one of the fixes broken out of patch 10000 that was
> > > missed in the merger. With this fix the CERROR called in
> > > sfw_handle_server_rpc will print out correctly.
> > Speaking of CERROR and logging, it it really useful
> > for each CERROR use to have 2 static structs?
> > 
> > In CERROR -> CDEBUG_LIMIT there is a:
> > 	static struct cfs_debug_limit_state cdls;
> > 	(12 or 16 bytes depending on 32/64 bit arch)
> > 
> > and in CDEBUG_LIMIT -> _CDEBUG
> > 	static struct libcfs_debug_msg_data msgdata;
> > 	(24 or 36 bytes depending on 32/64 bit arch)
> > 
> > That seems a largish bit of data and code to initialize
> > these structs for over a thousand call sites.
> > 
> > Wouldn't a single static suffice?
> Single static would not work because the code is parallel so it'll
> stomp over each other.

Sure, but would that matter in practice?

net_ratelimit() has similar parallelization and it doesn't
seem to matter there.

>  or do you mean to have a common
> structure for every callsite (but instantiated separately)?

That might help a tiny bit.

Some possibly unnecessary bits:

o .msg_cdls
o __FILE__, __func__ and __LINE__ fields have marginal value
o .msg_subsys seems set only to DEBUG_SUBSYSTEM.

> This used to be a local structure in the past, but that
> caused considerable stack growth for some functions, that added
> up along the call chain, and that was the solution we came up with
> that did help.

Re: [PATCH 1/6] staging: lustre: Correct missing newline for CERROR call in sfw_handle_server_rpc

[Drokin, Oleg]

On Mar 12, 2016, at 1:56 PM, Joe Perches wrote:

> On Sat, 2016-03-12 at 18:32 +0000, Drokin, Oleg wrote:
>> On Mar 12, 2016, at 1:23 PM, Joe Perches wrote:
>>> On Sat, 2016-03-12 at 13:00 -0500, James Simmons wrote:
>>>> From: James Nunez <james.a.nunez@intel.com>
>>>> 
>>>> This is one of the fixes broken out of patch 10000 that was
>>>> missed in the merger. With this fix the CERROR called in
>>>> sfw_handle_server_rpc will print out correctly.
>>> Speaking of CERROR and logging, it it really useful
>>> for each CERROR use to have 2 static structs?
>>> 
>>> In CERROR -> CDEBUG_LIMIT there is a:
>>> 	static struct cfs_debug_limit_state cdls;
>>> 	(12 or 16 bytes depending on 32/64 bit arch)
>>> 
>>> and in CDEBUG_LIMIT -> _CDEBUG
>>> 	static struct libcfs_debug_msg_data msgdata;
>>> 	(24 or 36 bytes depending on 32/64 bit arch)
>>> 
>>> That seems a largish bit of data and code to initialize
>>> these structs for over a thousand call sites.
>>> 
>>> Wouldn't a single static suffice?
>> Single static would not work because the code is parallel so it'll
>> stomp over each other.
> 
> Sure, but would that matter in practice?

Well. The bits about the callsite would certainly matter since
we need to know where the message is coming from.
Overwriting them in a racy way would make the messages unreliable.

> net_ratelimit() has similar parallelization and it doesn't
> seem to matter there.

That one seems to rate limit all prints together.
We are trying limit each individual one.
So if you have a bunch of print1 and a bunch of print2, but a few
of print3, you see the print3, but ratelimit the first two
and get something like this in the logs:

print1
print2
print3
print2 condensed: the message was repeated a gazillion times
print3
print1 condensed: the message was repeated two gazillion times.
> 
>> or do you mean to have a common
>> structure for every callsite (but instantiated separately)?
> 
> That might help a tiny bit.
> 
> Some possibly unnecessary bits:
> 
> o .msg_cdls

How are we going to rate-limit this stuff without remembering some
information between the calls?

> o __FILE__, __func__ and __LINE__ fields have marginal value

Probably not as important in the kernel indeed, but on the
other hand if the message has moved compared to the source developer has
then there is evidence some patches were applied and that could be asked
about.

> o .msg_subsys seems set only to DEBUG_SUBSYSTEM.

This is redefined in every source file:
drivers/staging/lustre/lustre/fid/fid_lib.c:#define DEBUG_SUBSYSTEM S_FID
drivers/staging/lustre/lustre/fid/fid_request.c:#define DEBUG_SUBSYSTEM S_FID
drivers/staging/lustre/lustre/fid/lproc_fid.c:#define DEBUG_SUBSYSTEM S_FID
drivers/staging/lustre/lustre/fld/fld_cache.c:#define DEBUG_SUBSYSTEM S_FLD
drivers/staging/lustre/lustre/fld/fld_request.c:#define DEBUG_SUBSYSTEM S_FLD
drivers/staging/lustre/lustre/fld/lproc_fld.c:#define DEBUG_SUBSYSTEM S_FLD
…

Allows you to filter out messages by subsystem in addition to by level.

Re: [PATCH 1/6] staging: lustre: Correct missing newline for CERROR call in sfw_handle_server_rpc

[Joe Perches]

On Sat, 2016-03-12 at 19:17 +0000, Drokin, Oleg wrote:
> On Mar 12, 2016, at 1:56 PM, Joe Perches wrote:
> > On Sat, 2016-03-12 at 18:32 +0000, Drokin, Oleg wrote:
> > > On Mar 12, 2016, at 1:23 PM, Joe Perches wrote:
> > > > On Sat, 2016-03-12 at 13:00 -0500, James Simmons wrote:
> > > > > From: James Nunez <james.a.nunez@intel.com>
> > > > > 
> > > > > This is one of the fixes broken out of patch 10000 that was
> > > > > missed in the merger. With this fix the CERROR called in
> > > > > sfw_handle_server_rpc will print out correctly.
> > > > Speaking of CERROR and logging, it it really useful
> > > > for each CERROR use to have 2 static structs?
> > > > 
> > > > In CERROR -> CDEBUG_LIMIT there is a:
> > > > 	static struct cfs_debug_limit_state cdls;
> > > > 	(12 or 16 bytes depending on 32/64 bit arch)
> > > > 
> > > > and in CDEBUG_LIMIT -> _CDEBUG
> > > > 	static struct libcfs_debug_msg_data msgdata;
> > > > 	(24 or 36 bytes depending on 32/64 bit arch)
> > > > 
> > > > That seems a largish bit of data and code to initialize
> > > > these structs for over a thousand call sites.
> > > > 
> > > > Wouldn't a single static suffice?
> > > Single static would not work because the code is parallel so it'll
> > > stomp over each other.
> > Sure, but would that matter in practice?
> Well. The bits about the callsite would certainly matter since
> we need to know where the message is coming from.
> Overwriting them in a racy way would make the messages unreliable.
> > net_ratelimit() has similar parallelization and it doesn't
> > seem to matter there.
> That one seems to rate limit all prints together.
> We are trying limit each individual one.
> So if you have a bunch of print1 and a bunch of print2, but a few
> of print3, you see the print3, but ratelimit the first two
> and get something like this in the logs:
> 
> print1
> print2
> print3
> print2 condensed: the message was repeated a gazillion times
> print3
> print1 condensed: the message was repeated two gazillion times.

Sure.  It's up to you to control your output and
I don't know if it matters or not.  You do.

> > > or do you mean to have a common
> > > structure for every callsite (but instantiated separately)?
> > That might help a tiny bit.
> > 
> > Some possibly unnecessary bits:
> > 
> > o .msg_cdls
> How are we going to rate-limit this stuff without remembering some
> information between the calls?

Doesn't msg_cdls just point to the other structure?
Combining the 2 into one might be useful.

> > o __FILE__, __func__ and __LINE__ fields have marginal value
> Probably not as important in the kernel indeed, but on the
> other hand if the message has moved compared to the source developer has
> then there is evidence some patches were applied and that could be asked
> about.

> > o .msg_subsys seems set only to DEBUG_SUBSYSTEM.
> This is redefined in every source file:

Thanks.  I didn't look hard.