diff mbox series

[23/29] lustre: osc_cache: remove 'transient' arg from osc_enter_cache_try

Message ID 154701504254.26726.7702205638412400024.stgit@noble (mailing list archive)
State New, archived
Headers show
Series assorted osc cleanups. | expand

Commit Message

NeilBrown Jan. 9, 2019, 6:24 a.m. UTC
This arg is always '0', so remove it.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 drivers/staging/lustre/lustre/osc/osc_cache.c |   11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

Comments

Andreas Dilger Jan. 10, 2019, 3:02 a.m. UTC | #1
On Jan 8, 2019, at 23:24, NeilBrown <neilb@suse.com> wrote:
> 
> This arg is always '0', so remove it.
> 
> Signed-off-by: NeilBrown <neilb@suse.com>

Out of curiosity, how would you find something like this?  Just
through code reading, or do you have some sort of static analysis
tool that shows this is dead code?

Digging into this a bit more, it looks like this is the only place
that increments cl_dirty_transit and sets OBD_BRW_NOCACHE, which
means the corresponding code in osc_release_write_grant() that checks
OBD_BRW_NOCACHE and decrements cl_dirty_transit is also dead, which
is good otherwise there would have been some kind of accounting leak.

That further implies that cl_dirty_transit is unused and can be removed,
along with obd_dirty_transit_pages.  The OBD_BRW_NOCACHE flag is part
of the wire protocol, but it looks like this was never actually sent on
the wire, just an internal housekeeping flag, so it should be marked like:

/* #define OBD_BRW_NOCACHE		0x400 internal use only 2.2.57-2.12 */

The follow-on cleanup could be part of this patch or a separate one.

Reviewed-by: Andreas Dilger <adilger@whamcloud.com>

> ---
> drivers/staging/lustre/lustre/osc/osc_cache.c |   11 +++--------
> 1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/staging/lustre/lustre/osc/osc_cache.c b/drivers/staging/lustre/lustre/osc/osc_cache.c
> index 8a68d3eb9314..b4bb36926046 100644
> --- a/drivers/staging/lustre/lustre/osc/osc_cache.c
> +++ b/drivers/staging/lustre/lustre/osc/osc_cache.c
> @@ -1535,7 +1535,7 @@ static void osc_exit_cache(struct client_obd *cli, struct osc_async_page *oap)
>  */
> static bool osc_enter_cache_try(struct client_obd *cli,
> 				struct osc_async_page *oap,
> -				int bytes, int transient)
> +				int bytes)
> {
> 	OSC_DUMP_GRANT(D_CACHE, cli, "need:%d\n", bytes);
> 
> @@ -1545,11 +1545,6 @@ static bool osc_enter_cache_try(struct client_obd *cli,
> 	if (cli->cl_dirty_pages < cli->cl_dirty_max_pages &&
> 	    atomic_long_read(&obd_dirty_pages) + 1 <= obd_max_dirty_pages) {
> 		osc_consume_write_grant(cli, &oap->oap_brw_page);
> -		if (transient) {
> -			cli->cl_dirty_transit++;
> -			atomic_long_inc(&obd_dirty_transit_pages);
> -			oap->oap_brw_flags |= OBD_BRW_NOCACHE;
> -		}
> 		return true;
> 	} else {
> 		__osc_unreserve_grant(cli, bytes, bytes);
> @@ -1618,7 +1613,7 @@ static int osc_enter_cache(const struct lu_env *env, struct client_obd *cli,
> 	remain = wait_event_idle_exclusive_timeout_cmd(
> 		cli->cl_cache_waiters,
> 		(entered = osc_enter_cache_try(
> -			cli, oap, bytes, 0)) ||
> +			cli, oap, bytes)) ||
> 		(cli->cl_dirty_pages == 0 &&
> 		 cli->cl_w_in_flight == 0),
> 		timeout,
> @@ -2396,7 +2391,7 @@ int osc_queue_async_io(const struct lu_env *env, struct cl_io *io,
> 
> 		/* it doesn't need any grant to dirty this page */
> 		spin_lock(&cli->cl_loi_list_lock);
> -		if (!osc_enter_cache_try(cli, oap, grants, 0)) {
> +		if (!osc_enter_cache_try(cli, oap, grants)) {
> 			grants = 0;
> 			need_release = 1;
> 		}
> 
> 

Cheers, Andreas
---
Andreas Dilger
CTO Whamcloud
NeilBrown Jan. 10, 2019, 4:04 a.m. UTC | #2
On Thu, Jan 10 2019, Andreas Dilger wrote:

> On Jan 8, 2019, at 23:24, NeilBrown <neilb@suse.com> wrote:
>> 
>> This arg is always '0', so remove it.
>> 
>> Signed-off-by: NeilBrown <neilb@suse.com>
>
> Out of curiosity, how would you find something like this?  Just
> through code reading, or do you have some sort of static analysis
> tool that shows this is dead code?

Just through code inspection - exactly as you demonstrate below :-)

I'll add the changes you suggest, probably all in the one patch if that
ends up looking OK.

Thanks,
NeilBrown


>
> Digging into this a bit more, it looks like this is the only place
> that increments cl_dirty_transit and sets OBD_BRW_NOCACHE, which
> means the corresponding code in osc_release_write_grant() that checks
> OBD_BRW_NOCACHE and decrements cl_dirty_transit is also dead, which
> is good otherwise there would have been some kind of accounting leak.
>
> That further implies that cl_dirty_transit is unused and can be removed,
> along with obd_dirty_transit_pages.  The OBD_BRW_NOCACHE flag is part
> of the wire protocol, but it looks like this was never actually sent on
> the wire, just an internal housekeeping flag, so it should be marked like:
>
> /* #define OBD_BRW_NOCACHE		0x400 internal use only 2.2.57-2.12 */
>
> The follow-on cleanup could be part of this patch or a separate one.
>
> Reviewed-by: Andreas Dilger <adilger@whamcloud.com>
>
>> ---
>> drivers/staging/lustre/lustre/osc/osc_cache.c |   11 +++--------
>> 1 file changed, 3 insertions(+), 8 deletions(-)
>> 
>> diff --git a/drivers/staging/lustre/lustre/osc/osc_cache.c b/drivers/staging/lustre/lustre/osc/osc_cache.c
>> index 8a68d3eb9314..b4bb36926046 100644
>> --- a/drivers/staging/lustre/lustre/osc/osc_cache.c
>> +++ b/drivers/staging/lustre/lustre/osc/osc_cache.c
>> @@ -1535,7 +1535,7 @@ static void osc_exit_cache(struct client_obd *cli, struct osc_async_page *oap)
>>  */
>> static bool osc_enter_cache_try(struct client_obd *cli,
>> 				struct osc_async_page *oap,
>> -				int bytes, int transient)
>> +				int bytes)
>> {
>> 	OSC_DUMP_GRANT(D_CACHE, cli, "need:%d\n", bytes);
>> 
>> @@ -1545,11 +1545,6 @@ static bool osc_enter_cache_try(struct client_obd *cli,
>> 	if (cli->cl_dirty_pages < cli->cl_dirty_max_pages &&
>> 	    atomic_long_read(&obd_dirty_pages) + 1 <= obd_max_dirty_pages) {
>> 		osc_consume_write_grant(cli, &oap->oap_brw_page);
>> -		if (transient) {
>> -			cli->cl_dirty_transit++;
>> -			atomic_long_inc(&obd_dirty_transit_pages);
>> -			oap->oap_brw_flags |= OBD_BRW_NOCACHE;
>> -		}
>> 		return true;
>> 	} else {
>> 		__osc_unreserve_grant(cli, bytes, bytes);
>> @@ -1618,7 +1613,7 @@ static int osc_enter_cache(const struct lu_env *env, struct client_obd *cli,
>> 	remain = wait_event_idle_exclusive_timeout_cmd(
>> 		cli->cl_cache_waiters,
>> 		(entered = osc_enter_cache_try(
>> -			cli, oap, bytes, 0)) ||
>> +			cli, oap, bytes)) ||
>> 		(cli->cl_dirty_pages == 0 &&
>> 		 cli->cl_w_in_flight == 0),
>> 		timeout,
>> @@ -2396,7 +2391,7 @@ int osc_queue_async_io(const struct lu_env *env, struct cl_io *io,
>> 
>> 		/* it doesn't need any grant to dirty this page */
>> 		spin_lock(&cli->cl_loi_list_lock);
>> -		if (!osc_enter_cache_try(cli, oap, grants, 0)) {
>> +		if (!osc_enter_cache_try(cli, oap, grants)) {
>> 			grants = 0;
>> 			need_release = 1;
>> 		}
>> 
>> 
>
> Cheers, Andreas
> ---
> Andreas Dilger
> CTO Whamcloud
NeilBrown Jan. 11, 2019, 12:27 a.m. UTC | #3
On Thu, Jan 10 2019, Andreas Dilger wrote:

> On Jan 8, 2019, at 23:24, NeilBrown <neilb@suse.com> wrote:
>> 
>> This arg is always '0', so remove it.
>> 
>> Signed-off-by: NeilBrown <neilb@suse.com>
>
> Out of curiosity, how would you find something like this?  Just
> through code reading, or do you have some sort of static analysis
> tool that shows this is dead code?
>
> Digging into this a bit more, it looks like this is the only place
> that increments cl_dirty_transit and sets OBD_BRW_NOCACHE, which
> means the corresponding code in osc_release_write_grant() that checks
> OBD_BRW_NOCACHE and decrements cl_dirty_transit is also dead, which
> is good otherwise there would have been some kind of accounting leak.
>
> That further implies that cl_dirty_transit is unused and can be removed,
> along with obd_dirty_transit_pages.  The OBD_BRW_NOCACHE flag is part
> of the wire protocol, but it looks like this was never actually sent on
> the wire, just an internal housekeeping flag, so it should be marked like:
>
> /* #define OBD_BRW_NOCACHE		0x400 internal use only 2.2.57-2.12 */
>
> The follow-on cleanup could be part of this patch or a separate one.
>
> Reviewed-by: Andreas Dilger <adilger@whamcloud.com>

Below is the revised version of this patch.
Thanks,
NeilBrown

From: NeilBrown <neilb@suse.com>
Subject: [PATCH] lustre: osc_cache: remove 'transient' arg from
 osc_enter_cache_try

This arg is always '0', so remove it.
Consequently, OBD_BRW_NOCACHE is never set, and
 cl_dirty_transit and obd_dirty_transit_pages
are never non-zero.
So they can be removed as well.

Reviewed-by: Andreas Dilger <adilger@whamcloud.com>
Signed-off-by: NeilBrown <neilb@suse.com>
---
 .../lustre/include/uapi/linux/lustre/lustre_idl.h        |  2 +-
 drivers/staging/lustre/lustre/include/obd.h              |  1 -
 drivers/staging/lustre/lustre/include/obd_support.h      |  1 -
 drivers/staging/lustre/lustre/obdclass/class_obd.c       |  3 ---
 drivers/staging/lustre/lustre/osc/osc_cache.c            | 16 +++-------------
 drivers/staging/lustre/lustre/osc/osc_request.c          | 14 ++++++--------
 drivers/staging/lustre/lustre/ptlrpc/wiretest.c          |  2 --
 7 files changed, 10 insertions(+), 29 deletions(-)

diff --git a/drivers/staging/lustre/include/uapi/linux/lustre/lustre_idl.h b/drivers/staging/lustre/include/uapi/linux/lustre/lustre_idl.h
index a8de36c8fbe4..bffe62e87e00 100644
--- a/drivers/staging/lustre/include/uapi/linux/lustre/lustre_idl.h
+++ b/drivers/staging/lustre/include/uapi/linux/lustre/lustre_idl.h
@@ -1182,7 +1182,7 @@ struct hsm_state_set {
 #define OBD_BRW_CHECK		0x10
 #define OBD_BRW_FROM_GRANT	0x20 /* the osc manages this under llite */
 #define OBD_BRW_GRANTED		0x40 /* the ost manages this */
-#define OBD_BRW_NOCACHE		0x80 /* this page is a part of non-cached IO */
+/* #define OBD_BRW_NOCACHE	0x80 internal use only 2.2.57-2.12 */
 #define OBD_BRW_NOQUOTA	       0x100
 #define OBD_BRW_SRVLOCK	       0x200 /* Client holds no lock over this page */
 #define OBD_BRW_ASYNC	       0x400 /* Server may delay commit to disk */
diff --git a/drivers/staging/lustre/lustre/include/obd.h b/drivers/staging/lustre/lustre/include/obd.h
index 0d8b9fe4bcec..4b43707f3d36 100644
--- a/drivers/staging/lustre/lustre/include/obd.h
+++ b/drivers/staging/lustre/lustre/include/obd.h
@@ -193,7 +193,6 @@ struct client_obd {
 	/* the grant values are protected by loi_list_lock below */
 	unsigned long		 cl_dirty_pages;	/* all _dirty_ in pages */
 	unsigned long		 cl_dirty_max_pages;	/* allowed w/o rpc */
-	unsigned long		 cl_dirty_transit;	/* dirty synchronous */
 	unsigned long		 cl_avail_grant;	/* bytes of credit for ost */
 	unsigned long		 cl_lost_grant;		/* lost credits (trunc) */
 	/* grant consumed for dirty pages */
diff --git a/drivers/staging/lustre/lustre/include/obd_support.h b/drivers/staging/lustre/lustre/include/obd_support.h
index 79875fad3f18..93a374514a77 100644
--- a/drivers/staging/lustre/lustre/include/obd_support.h
+++ b/drivers/staging/lustre/lustre/include/obd_support.h
@@ -55,7 +55,6 @@ extern int at_early_margin;
 extern int at_extra;
 extern unsigned long obd_max_dirty_pages;
 extern atomic_long_t obd_dirty_pages;
-extern atomic_long_t obd_dirty_transit_pages;
 extern char obd_jobid_var[];
 
 /* Some hash init argument constants */
diff --git a/drivers/staging/lustre/lustre/obdclass/class_obd.c b/drivers/staging/lustre/lustre/obdclass/class_obd.c
index 5b0b2f64a4a3..e563ebb5b328 100644
--- a/drivers/staging/lustre/lustre/obdclass/class_obd.c
+++ b/drivers/staging/lustre/lustre/obdclass/class_obd.c
@@ -77,9 +77,6 @@ EXPORT_SYMBOL(at_early_margin);
 int at_extra = 30;
 EXPORT_SYMBOL(at_extra);
 
-atomic_long_t obd_dirty_transit_pages;
-EXPORT_SYMBOL(obd_dirty_transit_pages);
-
 char obd_jobid_var[JOBSTATS_JOBID_VAR_MAX_LEN + 1] = JOBSTATS_DISABLE;
 char obd_jobid_node[LUSTRE_JOBID_SIZE + 1];
 
diff --git a/drivers/staging/lustre/lustre/osc/osc_cache.c b/drivers/staging/lustre/lustre/osc/osc_cache.c
index 8f5c567b4e15..53f067d4e09b 100644
--- a/drivers/staging/lustre/lustre/osc/osc_cache.c
+++ b/drivers/staging/lustre/lustre/osc/osc_cache.c
@@ -1424,11 +1424,6 @@ static void osc_release_write_grant(struct client_obd *cli,
 	pga->flag &= ~OBD_BRW_FROM_GRANT;
 	atomic_long_dec(&obd_dirty_pages);
 	cli->cl_dirty_pages--;
-	if (pga->flag & OBD_BRW_NOCACHE) {
-		pga->flag &= ~OBD_BRW_NOCACHE;
-		atomic_long_dec(&obd_dirty_transit_pages);
-		cli->cl_dirty_transit--;
-	}
 }
 
 /**
@@ -1535,7 +1530,7 @@ static void osc_exit_cache(struct client_obd *cli, struct osc_async_page *oap)
  */
 static bool osc_enter_cache_try(struct client_obd *cli,
 				struct osc_async_page *oap,
-				int bytes, int transient)
+				int bytes)
 {
 	OSC_DUMP_GRANT(D_CACHE, cli, "need:%d\n", bytes);
 
@@ -1545,11 +1540,6 @@ static bool osc_enter_cache_try(struct client_obd *cli,
 	if (cli->cl_dirty_pages < cli->cl_dirty_max_pages &&
 	    atomic_long_read(&obd_dirty_pages) + 1 <= obd_max_dirty_pages) {
 		osc_consume_write_grant(cli, &oap->oap_brw_page);
-		if (transient) {
-			cli->cl_dirty_transit++;
-			atomic_long_inc(&obd_dirty_transit_pages);
-			oap->oap_brw_flags |= OBD_BRW_NOCACHE;
-		}
 		return true;
 	} else {
 		__osc_unreserve_grant(cli, bytes, bytes);
@@ -1618,7 +1608,7 @@ static int osc_enter_cache(const struct lu_env *env, struct client_obd *cli,
 	remain = wait_event_idle_exclusive_timeout_cmd(
 		cli->cl_cache_waiters,
 		(entered = osc_enter_cache_try(
-			cli, oap, bytes, 0)) ||
+			cli, oap, bytes)) ||
 		(cli->cl_dirty_pages == 0 &&
 		 cli->cl_w_in_flight == 0),
 		timeout,
@@ -2396,7 +2386,7 @@ int osc_queue_async_io(const struct lu_env *env, struct cl_io *io,
 
 		/* it doesn't need any grant to dirty this page */
 		spin_lock(&cli->cl_loi_list_lock);
-		if (!osc_enter_cache_try(cli, oap, grants, 0)) {
+		if (!osc_enter_cache_try(cli, oap, grants)) {
 			grants = 0;
 			need_release = 1;
 		}
diff --git a/drivers/staging/lustre/lustre/osc/osc_request.c b/drivers/staging/lustre/lustre/osc/osc_request.c
index b28fbacbcfbf..e18d592bf42a 100644
--- a/drivers/staging/lustre/lustre/osc/osc_request.c
+++ b/drivers/staging/lustre/lustre/osc/osc_request.c
@@ -678,22 +678,20 @@ static void osc_announce_cached(struct client_obd *cli, struct obdo *oa,
 		oa->o_dirty = cli->cl_dirty_grant;
 	else
 		oa->o_dirty = cli->cl_dirty_pages << PAGE_SHIFT;
-	if (unlikely(cli->cl_dirty_pages - cli->cl_dirty_transit >
+	if (unlikely(cli->cl_dirty_pages >
 		     cli->cl_dirty_max_pages)) {
-		CERROR("dirty %lu - %lu > dirty_max %lu\n",
-		       cli->cl_dirty_pages, cli->cl_dirty_transit,
+		CERROR("dirty %lu > dirty_max %lu\n",
+		       cli->cl_dirty_pages,
 		       cli->cl_dirty_max_pages);
 		oa->o_undirty = 0;
-	} else if (unlikely(atomic_long_read(&obd_dirty_pages) -
-			    atomic_long_read(&obd_dirty_transit_pages) >
+	} else if (unlikely(atomic_long_read(&obd_dirty_pages) >
 			    (long)(obd_max_dirty_pages + 1))) {
 		/* The atomic_read() allowing the atomic_inc() are
 		 * not covered by a lock thus they may safely race and trip
 		 * this CERROR() unless we add in a small fudge factor (+1).
 		 */
-		CERROR("%s: dirty %ld + %ld > system dirty_max %ld\n",
+		CERROR("%s: dirty %ld > system dirty_max %ld\n",
 		       cli_name(cli), atomic_long_read(&obd_dirty_pages),
-		       atomic_long_read(&obd_dirty_transit_pages),
 		       obd_max_dirty_pages);
 		oa->o_undirty = 0;
 	} else if (unlikely(cli->cl_dirty_max_pages - cli->cl_dirty_pages >
@@ -1047,7 +1045,7 @@ static int check_write_rcs(struct ptlrpc_request *req,
 static inline int can_merge_pages(struct brw_page *p1, struct brw_page *p2)
 {
 	if (p1->flag != p2->flag) {
-		unsigned int mask = ~(OBD_BRW_FROM_GRANT | OBD_BRW_NOCACHE |
+		unsigned int mask = ~(OBD_BRW_FROM_GRANT |
 				      OBD_BRW_SYNC | OBD_BRW_ASYNC |
 				      OBD_BRW_NOQUOTA | OBD_BRW_SOFT_SYNC);
 
diff --git a/drivers/staging/lustre/lustre/ptlrpc/wiretest.c b/drivers/staging/lustre/lustre/ptlrpc/wiretest.c
index 639db24b4f63..c2f49a3e0454 100644
--- a/drivers/staging/lustre/lustre/ptlrpc/wiretest.c
+++ b/drivers/staging/lustre/lustre/ptlrpc/wiretest.c
@@ -1823,8 +1823,6 @@ void lustre_assert_wire_constants(void)
 		 OBD_BRW_FROM_GRANT);
 	LASSERTF(OBD_BRW_GRANTED == 0x40, "found 0x%.8x\n",
 		 OBD_BRW_GRANTED);
-	LASSERTF(OBD_BRW_NOCACHE == 0x80, "found 0x%.8x\n",
-		 OBD_BRW_NOCACHE);
 	LASSERTF(OBD_BRW_NOQUOTA == 0x100, "found 0x%.8x\n",
 		 OBD_BRW_NOQUOTA);
 	LASSERTF(OBD_BRW_SRVLOCK == 0x200, "found 0x%.8x\n",
diff mbox series

Patch

diff --git a/drivers/staging/lustre/lustre/osc/osc_cache.c b/drivers/staging/lustre/lustre/osc/osc_cache.c
index 8a68d3eb9314..b4bb36926046 100644
--- a/drivers/staging/lustre/lustre/osc/osc_cache.c
+++ b/drivers/staging/lustre/lustre/osc/osc_cache.c
@@ -1535,7 +1535,7 @@  static void osc_exit_cache(struct client_obd *cli, struct osc_async_page *oap)
  */
 static bool osc_enter_cache_try(struct client_obd *cli,
 				struct osc_async_page *oap,
-				int bytes, int transient)
+				int bytes)
 {
 	OSC_DUMP_GRANT(D_CACHE, cli, "need:%d\n", bytes);
 
@@ -1545,11 +1545,6 @@  static bool osc_enter_cache_try(struct client_obd *cli,
 	if (cli->cl_dirty_pages < cli->cl_dirty_max_pages &&
 	    atomic_long_read(&obd_dirty_pages) + 1 <= obd_max_dirty_pages) {
 		osc_consume_write_grant(cli, &oap->oap_brw_page);
-		if (transient) {
-			cli->cl_dirty_transit++;
-			atomic_long_inc(&obd_dirty_transit_pages);
-			oap->oap_brw_flags |= OBD_BRW_NOCACHE;
-		}
 		return true;
 	} else {
 		__osc_unreserve_grant(cli, bytes, bytes);
@@ -1618,7 +1613,7 @@  static int osc_enter_cache(const struct lu_env *env, struct client_obd *cli,
 	remain = wait_event_idle_exclusive_timeout_cmd(
 		cli->cl_cache_waiters,
 		(entered = osc_enter_cache_try(
-			cli, oap, bytes, 0)) ||
+			cli, oap, bytes)) ||
 		(cli->cl_dirty_pages == 0 &&
 		 cli->cl_w_in_flight == 0),
 		timeout,
@@ -2396,7 +2391,7 @@  int osc_queue_async_io(const struct lu_env *env, struct cl_io *io,
 
 		/* it doesn't need any grant to dirty this page */
 		spin_lock(&cli->cl_loi_list_lock);
-		if (!osc_enter_cache_try(cli, oap, grants, 0)) {
+		if (!osc_enter_cache_try(cli, oap, grants)) {
 			grants = 0;
 			need_release = 1;
 		}