[lustre-devel] [PATCH 00/29] assorted osc cleanups.

[lustre-devel] [PATCH 00/29] assorted osc cleanups.

[NeilBrown]

I was reading through the osc code recently trying to understand it,
and tripped over various things that made it harded to understand.
So I wrote patches to clean those things up.  As you can see below,
this deleted nearly 300 lines of code.

None of these are bug-fixes, and some may only be subjective
improvements, so I can drop any that anyone objects to.

This seems to fit well with James' recent suggesting to focus on
house-cleaning for a while (which I support).

Thanks,
NeilBrown


---

NeilBrown (29):
      lustre: osc_cache: discard oe_intree
      lustre: osc_cache: use assert_spin_locked()
      lustre: osc: simplify osc_extent_wait()
      lustre: osc: simplify list manipulation
      lustre: osc: convert oe_refc and oe_users to kref and refcount_
      lustre: osc: use overlapped() consistently.
      lustre: osc: convert a while loop to for
      lustre: osc: simplify osc_extent_find()
      lustre: osc: remove test on 'found' being an error.
      lustre: osc_cache: avoid list_for_each_entry_safe when clearing list.
      lustre: osc_cache: simplify osc_wake_cache_waiters()
      lustre: osc_cache: avoid confusing variable reuse.
      lustre: osc_cache: change osc_enter_cache_try to return bool.
      lustre: osc_cache: convert cl_cache_waiters to a wait_queue.
      lustre: osc_cache: change osc_make_rpc() to return bool.
      lustre: osc_cache: use osc_makes_hprpc() more consistently.
      lustre: osc_cache: simplify list walk in get_write_extents().
      lustre: osc_cache: avoid unnecessary tests.
      lustre: osc_cache: convert while to for in get_write_extents()
      lustre: osc_cache: don't drop a lock we didn't take.
      lustre: osc_cache: don't drop a lock we didn't take - two
      lustre: osc_cache: osc_prep_async_page() has meaningless return
      lustre: osc_cache: remove 'transient' arg from osc_enter_cache_try
      lustre: osc_cache: change need_release to bool
      lustre: remove cl_page_cancel()
      lustre: osc_cache: simplify osc_page_gang_lookup()
      lustre: osc_cache: white-space and other checkpatch fixes.
      lustre: osc_request: assorted white-space and check-patch fixes.
      lustre: centralize handling of PTLRPCD_SET


 drivers/staging/lustre/lustre/include/cl_object.h  |   25 -
 drivers/staging/lustre/lustre/include/lustre_net.h |    2 
 drivers/staging/lustre/lustre/include/obd.h        |    2 
 drivers/staging/lustre/lustre/ldlm/ldlm_lib.c      |    2 
 drivers/staging/lustre/lustre/obdclass/cl_page.c   |   20 -
 drivers/staging/lustre/lustre/osc/osc_cache.c      |  722 +++++++-------------
 .../staging/lustre/lustre/osc/osc_cl_internal.h    |   33 -
 drivers/staging/lustre/lustre/osc/osc_internal.h   |   15 
 drivers/staging/lustre/lustre/osc/osc_io.c         |    4 
 drivers/staging/lustre/lustre/osc/osc_lock.c       |   27 -
 drivers/staging/lustre/lustre/osc/osc_page.c       |   22 -
 drivers/staging/lustre/lustre/osc/osc_request.c    |  199 +++---
 drivers/staging/lustre/lustre/ptlrpc/client.c      |   20 -
 13 files changed, 399 insertions(+), 694 deletions(-)

--
Signature

[lustre-devel] [PATCH 01/29] lustre: osc_cache: discard oe_intree

[NeilBrown]

An rbnode knows if it is in the tree or not, using RB_EMPTY_NODE().
There is no need for an extra flag.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 drivers/staging/lustre/lustre/osc/osc_cache.c      |   15 +++++++--------
 .../staging/lustre/lustre/osc/osc_cl_internal.h    |    3 +--
 2 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/lustre/lustre/osc/osc_cache.c b/drivers/staging/lustre/lustre/osc/osc_cache.c
index 93330cb77e94..fbf16547003d 100644
--- a/drivers/staging/lustre/lustre/osc/osc_cache.c
+++ b/drivers/staging/lustre/lustre/osc/osc_cache.c
@@ -74,7 +74,7 @@ static inline char *ext_flags(struct osc_extent *ext, char *flags)
 {
 	char *buf = flags;
 	*buf++ = ext->oe_rw ? 'r' : 'w';
-	if (ext->oe_intree)
+	if (!RB_EMPTY_NODE(&ext->oe_node))
 		*buf++ = 'i';
 	if (ext->oe_sync)
 		*buf++ = 'S';
@@ -154,7 +154,7 @@ static inline struct osc_extent *next_extent(struct osc_extent *ext)
 	if (!ext)
 		return NULL;
 
-	LASSERT(ext->oe_intree);
+	LASSERT(!RB_EMPTY_NODE(&ext->oe_node));
 	return rb_extent(rb_next(&ext->oe_node));
 }
 
@@ -163,7 +163,7 @@ static inline struct osc_extent *prev_extent(struct osc_extent *ext)
 	if (!ext)
 		return NULL;
 
-	LASSERT(ext->oe_intree);
+	LASSERT(!RB_EMPTY_NODE(&ext->oe_node));
 	return rb_extent(rb_prev(&ext->oe_node));
 }
 
@@ -393,7 +393,7 @@ static void osc_extent_put(const struct lu_env *env, struct osc_extent *ext)
 		LASSERT(list_empty(&ext->oe_link));
 		LASSERT(atomic_read(&ext->oe_users) == 0);
 		LASSERT(ext->oe_state == OES_INV);
-		LASSERT(!ext->oe_intree);
+		LASSERT(RB_EMPTY_NODE(&ext->oe_node));
 
 		if (ext->oe_dlmlock) {
 			lu_ref_add(&ext->oe_dlmlock->l_reference,
@@ -465,7 +465,7 @@ static void osc_extent_insert(struct osc_object *obj, struct osc_extent *ext)
 	struct rb_node *parent = NULL;
 	struct osc_extent *tmp;
 
-	LASSERT(ext->oe_intree == 0);
+	LASSERT(RB_EMPTY_NODE(&ext->oe_node));
 	LASSERT(ext->oe_obj == obj);
 	LASSERT(osc_object_is_locked(obj));
 	while (*n) {
@@ -482,7 +482,6 @@ static void osc_extent_insert(struct osc_object *obj, struct osc_extent *ext)
 	rb_link_node(&ext->oe_node, parent, n);
 	rb_insert_color(&ext->oe_node, &obj->oo_root);
 	osc_extent_get(ext);
-	ext->oe_intree = 1;
 }
 
 /* caller must have held object lock. */
@@ -491,9 +490,9 @@ static void osc_extent_erase(struct osc_extent *ext)
 	struct osc_object *obj = ext->oe_obj;
 
 	LASSERT(osc_object_is_locked(obj));
-	if (ext->oe_intree) {
+	if (!RB_EMPTY_NODE(&ext->oe_node)) {
 		rb_erase(&ext->oe_node, &obj->oo_root);
-		ext->oe_intree = 0;
+		RB_CLEAR_NODE(&ext->oe_node);
 		/* rbtree held a refcount */
 		osc_extent_put_trust(ext);
 	}
diff --git a/drivers/staging/lustre/lustre/osc/osc_cl_internal.h b/drivers/staging/lustre/lustre/osc/osc_cl_internal.h
index 077a2b183634..b78deef3963a 100644
--- a/drivers/staging/lustre/lustre/osc/osc_cl_internal.h
+++ b/drivers/staging/lustre/lustre/osc/osc_cl_internal.h
@@ -598,9 +598,8 @@ struct osc_extent {
 	/* state of this extent */
 	enum osc_extent_state	oe_state;
 	/* flags for this extent. */
-	unsigned int		oe_intree:1,
 	/* 0 is write, 1 is read */
-				oe_rw:1,
+	unsigned int		oe_rw:1,
 	/* sync extent, queued by osc_queue_sync_pages() */
 				oe_sync:1,
 	/* set if this extent has partial, sync pages.

[lustre-devel] [PATCH 01/29] lustre: osc_cache: discard oe_intree

[Andreas Dilger]

On Jan 8, 2019, at 23:24, NeilBrown <neilb@suse.com> wrote:
> 
> An rbnode knows if it is in the tree or not, using RB_EMPTY_NODE().
> There is no need for an extra flag.
> 
> Signed-off-by: NeilBrown <neilb@suse.com>

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

> ---
> drivers/staging/lustre/lustre/osc/osc_cache.c      |   15 +++++++--------
> .../staging/lustre/lustre/osc/osc_cl_internal.h    |    3 +--
> 2 files changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/staging/lustre/lustre/osc/osc_cache.c b/drivers/staging/lustre/lustre/osc/osc_cache.c
> index 93330cb77e94..fbf16547003d 100644
> --- a/drivers/staging/lustre/lustre/osc/osc_cache.c
> +++ b/drivers/staging/lustre/lustre/osc/osc_cache.c
> @@ -74,7 +74,7 @@ static inline char *ext_flags(struct osc_extent *ext, char *flags)
> {
> 	char *buf = flags;
> 	*buf++ = ext->oe_rw ? 'r' : 'w';
> -	if (ext->oe_intree)
> +	if (!RB_EMPTY_NODE(&ext->oe_node))
> 		*buf++ = 'i';
> 	if (ext->oe_sync)
> 		*buf++ = 'S';
> @@ -154,7 +154,7 @@ static inline struct osc_extent *next_extent(struct osc_extent *ext)
> 	if (!ext)
> 		return NULL;
> 
> -	LASSERT(ext->oe_intree);
> +	LASSERT(!RB_EMPTY_NODE(&ext->oe_node));
> 	return rb_extent(rb_next(&ext->oe_node));
> }
> 
> @@ -163,7 +163,7 @@ static inline struct osc_extent *prev_extent(struct osc_extent *ext)
> 	if (!ext)
> 		return NULL;
> 
> -	LASSERT(ext->oe_intree);
> +	LASSERT(!RB_EMPTY_NODE(&ext->oe_node));
> 	return rb_extent(rb_prev(&ext->oe_node));
> }
> 
> @@ -393,7 +393,7 @@ static void osc_extent_put(const struct lu_env *env, struct osc_extent *ext)
> 		LASSERT(list_empty(&ext->oe_link));
> 		LASSERT(atomic_read(&ext->oe_users) == 0);
> 		LASSERT(ext->oe_state == OES_INV);
> -		LASSERT(!ext->oe_intree);
> +		LASSERT(RB_EMPTY_NODE(&ext->oe_node));
> 
> 		if (ext->oe_dlmlock) {
> 			lu_ref_add(&ext->oe_dlmlock->l_reference,
> @@ -465,7 +465,7 @@ static void osc_extent_insert(struct osc_object *obj, struct osc_extent *ext)
> 	struct rb_node *parent = NULL;
> 	struct osc_extent *tmp;
> 
> -	LASSERT(ext->oe_intree == 0);
> +	LASSERT(RB_EMPTY_NODE(&ext->oe_node));
> 	LASSERT(ext->oe_obj == obj);
> 	LASSERT(osc_object_is_locked(obj));
> 	while (*n) {
> @@ -482,7 +482,6 @@ static void osc_extent_insert(struct osc_object *obj, struct osc_extent *ext)
> 	rb_link_node(&ext->oe_node, parent, n);
> 	rb_insert_color(&ext->oe_node, &obj->oo_root);
> 	osc_extent_get(ext);
> -	ext->oe_intree = 1;
> }
> 
> /* caller must have held object lock. */
> @@ -491,9 +490,9 @@ static void osc_extent_erase(struct osc_extent *ext)
> 	struct osc_object *obj = ext->oe_obj;
> 
> 	LASSERT(osc_object_is_locked(obj));
> -	if (ext->oe_intree) {
> +	if (!RB_EMPTY_NODE(&ext->oe_node)) {
> 		rb_erase(&ext->oe_node, &obj->oo_root);
> -		ext->oe_intree = 0;
> +		RB_CLEAR_NODE(&ext->oe_node);
> 		/* rbtree held a refcount */
> 		osc_extent_put_trust(ext);
> 	}
> diff --git a/drivers/staging/lustre/lustre/osc/osc_cl_internal.h b/drivers/staging/lustre/lustre/osc/osc_cl_internal.h
> index 077a2b183634..b78deef3963a 100644
> --- a/drivers/staging/lustre/lustre/osc/osc_cl_internal.h
> +++ b/drivers/staging/lustre/lustre/osc/osc_cl_internal.h
> @@ -598,9 +598,8 @@ struct osc_extent {
> 	/* state of this extent */
> 	enum osc_extent_state	oe_state;
> 	/* flags for this extent. */
> -	unsigned int		oe_intree:1,
> 	/* 0 is write, 1 is read */
> -				oe_rw:1,
> +	unsigned int		oe_rw:1,
> 	/* sync extent, queued by osc_queue_sync_pages() */
> 				oe_sync:1,
> 	/* set if this extent has partial, sync pages.
> 
> 

Cheers, Andreas
---
Andreas Dilger
Principal Lustre Architect
Whamcloud

[lustre-devel] [PATCH 06/29] lustre: osc: use overlapped() consistently.

[NeilBrown]

osc_extent_is_overlapped() open-codes exactly the test that
overlapped() performs.
So use overlapped() instead, to make the code more obviously
consistent.

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

diff --git a/drivers/staging/lustre/lustre/osc/osc_cache.c b/drivers/staging/lustre/lustre/osc/osc_cache.c
index cab1a4f99cc2..dd3c87124aa5 100644
--- a/drivers/staging/lustre/lustre/osc/osc_cache.c
+++ b/drivers/staging/lustre/lustre/osc/osc_cache.c
@@ -313,6 +313,11 @@ static int osc_extent_sanity_check0(struct osc_extent *ext,
 	__res;								\
 })
 
+static inline int overlapped(struct osc_extent *ex1, struct osc_extent *ex2)
+{
+	return !(ex1->oe_end < ex2->oe_start || ex2->oe_end < ex1->oe_start);
+}
+
 /**
  * sanity check - to make sure there is no overlapped extent in the tree.
  */
@@ -329,8 +334,7 @@ static int osc_extent_is_overlapped(struct osc_object *obj,
 	for (tmp = first_extent(obj); tmp; tmp = next_extent(tmp)) {
 		if (tmp == ext)
 			continue;
-		if (tmp->oe_end >= ext->oe_start &&
-		    tmp->oe_start <= ext->oe_end)
+		if (overlapped(tmp, ext))
 			return 1;
 	}
 	return 0;
@@ -655,11 +659,6 @@ void osc_extent_release(const struct lu_env *env, struct osc_extent *ext)
 	osc_extent_put(env, ext);
 }
 
-static inline int overlapped(struct osc_extent *ex1, struct osc_extent *ex2)
-{
-	return !(ex1->oe_end < ex2->oe_start || ex2->oe_end < ex1->oe_start);
-}
-
 /**
  * Find or create an extent which includes @index, core function to manage
  * extent tree.

[lustre-devel] [PATCH 06/29] lustre: osc: use overlapped() consistently.

[Andreas Dilger]

On Jan 8, 2019, at 23:24, NeilBrown <neilb@suse.com> wrote:
> 
> osc_extent_is_overlapped() open-codes exactly the test that
> overlapped() performs.
> So use overlapped() instead, to make the code more obviously
> consistent.
> 
> Signed-off-by: NeilBrown <neilb@suse.com>

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

> ---
> drivers/staging/lustre/lustre/osc/osc_cache.c |   13 ++++++-------
> 1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/staging/lustre/lustre/osc/osc_cache.c b/drivers/staging/lustre/lustre/osc/osc_cache.c
> index cab1a4f99cc2..dd3c87124aa5 100644
> --- a/drivers/staging/lustre/lustre/osc/osc_cache.c
> +++ b/drivers/staging/lustre/lustre/osc/osc_cache.c
> @@ -313,6 +313,11 @@ static int osc_extent_sanity_check0(struct osc_extent *ext,
> 	__res;								\
> })
> 
> +static inline int overlapped(struct osc_extent *ex1, struct osc_extent *ex2)
> +{
> +	return !(ex1->oe_end < ex2->oe_start || ex2->oe_end < ex1->oe_start);
> +}
> +
> /**
>  * sanity check - to make sure there is no overlapped extent in the tree.
>  */
> @@ -329,8 +334,7 @@ static int osc_extent_is_overlapped(struct osc_object *obj,
> 	for (tmp = first_extent(obj); tmp; tmp = next_extent(tmp)) {
> 		if (tmp == ext)
> 			continue;
> -		if (tmp->oe_end >= ext->oe_start &&
> -		    tmp->oe_start <= ext->oe_end)
> +		if (overlapped(tmp, ext))
> 			return 1;
> 	}
> 	return 0;
> @@ -655,11 +659,6 @@ void osc_extent_release(const struct lu_env *env, struct osc_extent *ext)
> 	osc_extent_put(env, ext);
> }
> 
> -static inline int overlapped(struct osc_extent *ex1, struct osc_extent *ex2)
> -{
> -	return !(ex1->oe_end < ex2->oe_start || ex2->oe_end < ex1->oe_start);
> -}
> -
> /**
>  * Find or create an extent which includes @index, core function to manage
>  * extent tree.
> 
> 

Cheers, Andreas
---
Andreas Dilger
CTO Whamcloud

[lustre-devel] [PATCH 09/29] lustre: osc: remove test on 'found' being an error.

[NeilBrown]

Found cannot be IS_ERR() at this point in the code, as it is only ever
assigned a value from osc_extent_hold() (or NULL).

So discard the test.

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

diff --git a/drivers/staging/lustre/lustre/osc/osc_cache.c b/drivers/staging/lustre/lustre/osc/osc_cache.c
index 848e440ae2a9..e65d917336b9 100644
--- a/drivers/staging/lustre/lustre/osc/osc_cache.c
+++ b/drivers/staging/lustre/lustre/osc/osc_cache.c
@@ -821,11 +821,9 @@ static struct osc_extent *osc_extent_find(const struct lu_env *env,
 	osc_extent_tree_dump(D_CACHE, obj);
 	if (found) {
 		LASSERT(!conflict);
-		if (!IS_ERR(found)) {
-			LASSERT(found->oe_dlmlock == cur->oe_dlmlock);
-			OSC_EXTENT_DUMP(D_CACHE, found,
-					"found caching ext for %lu.\n", index);
-		}
+		LASSERT(found->oe_dlmlock == cur->oe_dlmlock);
+		OSC_EXTENT_DUMP(D_CACHE, found,
+				"found caching ext for %lu.\n", index);
 	} else if (!conflict) {
 		/* create a new extent */
 		EASSERT(osc_extent_is_overlapped(obj, cur) == 0, cur);

[lustre-devel] [PATCH 09/29] lustre: osc: remove test on 'found' being an error.

[Andreas Dilger]

On Jan 8, 2019, at 23:24, NeilBrown <neilb@suse.com> wrote:
> 
> Found cannot be IS_ERR() at this point in the code, as it is only ever
> assigned a value from osc_extent_hold() (or NULL).
> 
> So discard the test.
> 
> Signed-off-by: NeilBrown <neilb@suse.com>

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

> ---
> drivers/staging/lustre/lustre/osc/osc_cache.c |    8 +++-----
> 1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/lustre/lustre/osc/osc_cache.c b/drivers/staging/lustre/lustre/osc/osc_cache.c
> index 848e440ae2a9..e65d917336b9 100644
> --- a/drivers/staging/lustre/lustre/osc/osc_cache.c
> +++ b/drivers/staging/lustre/lustre/osc/osc_cache.c
> @@ -821,11 +821,9 @@ static struct osc_extent *osc_extent_find(const struct lu_env *env,
> 	osc_extent_tree_dump(D_CACHE, obj);
> 	if (found) {
> 		LASSERT(!conflict);
> -		if (!IS_ERR(found)) {
> -			LASSERT(found->oe_dlmlock == cur->oe_dlmlock);
> -			OSC_EXTENT_DUMP(D_CACHE, found,
> -					"found caching ext for %lu.\n", index);
> -		}
> +		LASSERT(found->oe_dlmlock == cur->oe_dlmlock);
> +		OSC_EXTENT_DUMP(D_CACHE, found,
> +				"found caching ext for %lu.\n", index);
> 	} else if (!conflict) {
> 		/* create a new extent */
> 		EASSERT(osc_extent_is_overlapped(obj, cur) == 0, cur);
> 
> 

Cheers, Andreas
---
Andreas Dilger
Principal Lustre Architect
Whamcloud

[lustre-devel] [PATCH 05/29] lustre: osc: convert oe_refc and oe_users to kref and refcount_

[NeilBrown]

oe_refc is used like a kref, so make it one.
oe_users isn't quite, as it is initialised to 0, so make it a
refcount_t.

As cl_object_put() needs an 'env', we cannot quite use
kref_put() as intended.  Maybe that can be fixed one day.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 drivers/staging/lustre/lustre/osc/osc_cache.c      |   73 ++++++++++++--------
 .../staging/lustre/lustre/osc/osc_cl_internal.h    |    4 +
 2 files changed, 45 insertions(+), 32 deletions(-)

diff --git a/drivers/staging/lustre/lustre/osc/osc_cache.c b/drivers/staging/lustre/lustre/osc/osc_cache.c
index 6771675dd520..cab1a4f99cc2 100644
--- a/drivers/staging/lustre/lustre/osc/osc_cache.c
+++ b/drivers/staging/lustre/lustre/osc/osc_cache.c
@@ -114,8 +114,8 @@ static const char *oes_strings[] = {
 		/* ----- extent part 0 ----- */				      \
 		__ext, EXTPARA(__ext),					      \
 		/* ----- part 1 ----- */				      \
-		atomic_read(&__ext->oe_refc),				      \
-		atomic_read(&__ext->oe_users),				      \
+		kref_read(&__ext->oe_refc),				      \
+		refcount_read(&__ext->oe_users),			      \
 		list_empty_marker(&__ext->oe_link),			      \
 		oes_strings[__ext->oe_state], ext_flags(__ext, __buf),	      \
 		__ext->oe_obj,						      \
@@ -188,12 +188,12 @@ static int osc_extent_sanity_check0(struct osc_extent *ext,
 		goto out;
 	}
 
-	if (atomic_read(&ext->oe_refc) <= 0) {
+	if (kref_read(&ext->oe_refc) <= 0) {
 		rc = 20;
 		goto out;
 	}
 
-	if (atomic_read(&ext->oe_refc) < atomic_read(&ext->oe_users)) {
+	if (kref_read(&ext->oe_refc) < refcount_read(&ext->oe_users)) {
 		rc = 30;
 		goto out;
 	}
@@ -206,7 +206,7 @@ static int osc_extent_sanity_check0(struct osc_extent *ext,
 			rc = 0;
 		goto out;
 	case OES_ACTIVE:
-		if (atomic_read(&ext->oe_users) == 0) {
+		if (refcount_read(&ext->oe_users) == 0) {
 			rc = 40;
 			goto out;
 		}
@@ -230,7 +230,7 @@ static int osc_extent_sanity_check0(struct osc_extent *ext,
 		}
 		/* fall through */
 	default:
-		if (atomic_read(&ext->oe_users) > 0) {
+		if (refcount_read(&ext->oe_users) > 0) {
 			rc = 70;
 			goto out;
 		}
@@ -360,8 +360,8 @@ static struct osc_extent *osc_extent_alloc(struct osc_object *obj)
 	RB_CLEAR_NODE(&ext->oe_node);
 	ext->oe_obj = obj;
 	cl_object_get(osc2cl(obj));
-	atomic_set(&ext->oe_refc, 1);
-	atomic_set(&ext->oe_users, 0);
+	kref_init(&ext->oe_refc);
+	refcount_set(&ext->oe_users, 0);
 	INIT_LIST_HEAD(&ext->oe_link);
 	ext->oe_state = OES_INV;
 	INIT_LIST_HEAD(&ext->oe_pages);
@@ -371,35 +371,48 @@ static struct osc_extent *osc_extent_alloc(struct osc_object *obj)
 	return ext;
 }
 
-static void osc_extent_free(struct osc_extent *ext)
+static void osc_extent_free(struct kref *kref)
 {
+	struct osc_extent *ext = container_of(kref, struct osc_extent,
+					      oe_refc);
+
+	LASSERT(list_empty(&ext->oe_link));
+	LASSERT(refcount_read(&ext->oe_users) == 0);
+	LASSERT(ext->oe_state == OES_INV);
+	LASSERT(RB_EMPTY_NODE(&ext->oe_node));
+
+	if (ext->oe_dlmlock) {
+		lu_ref_add(&ext->oe_dlmlock->l_reference,
+			   "osc_extent", ext);
+		LDLM_LOCK_PUT(ext->oe_dlmlock);
+		ext->oe_dlmlock = NULL;
+	}
+#if 0
+	// When cl_object_put drop the need for 'env',
+	// this code can be enabled.
+	cl_object_put(osc2cl(ext->oe_obj));
+
 	kmem_cache_free(osc_extent_kmem, ext);
+#endif
 }
 
 static struct osc_extent *osc_extent_get(struct osc_extent *ext)
 {
-	LASSERT(atomic_read(&ext->oe_refc) >= 0);
-	atomic_inc(&ext->oe_refc);
+	LASSERT(kref_read(&ext->oe_refc) >= 0);
+	kref_get(&ext->oe_refc);
 	return ext;
 }
 
 static void osc_extent_put(const struct lu_env *env, struct osc_extent *ext)
 {
-	LASSERT(atomic_read(&ext->oe_refc) > 0);
-	if (atomic_dec_and_test(&ext->oe_refc)) {
-		LASSERT(list_empty(&ext->oe_link));
-		LASSERT(atomic_read(&ext->oe_users) == 0);
-		LASSERT(ext->oe_state == OES_INV);
-		LASSERT(RB_EMPTY_NODE(&ext->oe_node));
-
-		if (ext->oe_dlmlock) {
-			lu_ref_add(&ext->oe_dlmlock->l_reference,
-				   "osc_extent", ext);
-			LDLM_LOCK_PUT(ext->oe_dlmlock);
-			ext->oe_dlmlock = NULL;
-		}
+	LASSERT(kref_read(&ext->oe_refc) > 0);
+	if (kref_put(&ext->oe_refc, osc_extent_free)) {
+		/* This should be in osc_extent_free(), but
+		 * while we need to pass 'env' it cannot be.
+		 */
 		cl_object_put(env, osc2cl(ext->oe_obj));
-		osc_extent_free(ext);
+
+		kmem_cache_free(osc_extent_kmem, ext);
 	}
 }
 
@@ -410,9 +423,9 @@ static void osc_extent_put(const struct lu_env *env, struct osc_extent *ext)
  */
 static void osc_extent_put_trust(struct osc_extent *ext)
 {
-	LASSERT(atomic_read(&ext->oe_refc) > 1);
+	LASSERT(kref_read(&ext->oe_refc) > 1);
 	assert_osc_object_is_locked(ext->oe_obj);
-	atomic_dec(&ext->oe_refc);
+	osc_extent_put(NULL, ext);
 }
 
 /**
@@ -505,7 +518,7 @@ static struct osc_extent *osc_extent_hold(struct osc_extent *ext)
 		osc_extent_state_set(ext, OES_ACTIVE);
 		osc_update_pending(obj, OBD_BRW_WRITE, -ext->oe_nr_pages);
 	}
-	atomic_inc(&ext->oe_users);
+	refcount_inc(&ext->oe_users);
 	list_del_init(&ext->oe_link);
 	return osc_extent_get(ext);
 }
@@ -599,11 +612,11 @@ void osc_extent_release(const struct lu_env *env, struct osc_extent *ext)
 	struct osc_object *obj = ext->oe_obj;
 	struct client_obd *cli = osc_cli(obj);
 
-	LASSERT(atomic_read(&ext->oe_users) > 0);
+	LASSERT(refcount_read(&ext->oe_users) > 0);
 	LASSERT(sanity_check(ext) == 0);
 	LASSERT(ext->oe_grants > 0);
 
-	if (atomic_dec_and_lock(&ext->oe_users, &obj->oo_lock)) {
+	if (refcount_dec_and_lock(&ext->oe_users, &obj->oo_lock)) {
 		LASSERT(ext->oe_state == OES_ACTIVE);
 		if (ext->oe_trunc_pending) {
 			/* a truncate process is waiting for this extent.
diff --git a/drivers/staging/lustre/lustre/osc/osc_cl_internal.h b/drivers/staging/lustre/lustre/osc/osc_cl_internal.h
index aa1b753fc88d..b1a1d241cc6c 100644
--- a/drivers/staging/lustre/lustre/osc/osc_cl_internal.h
+++ b/drivers/staging/lustre/lustre/osc/osc_cl_internal.h
@@ -579,9 +579,9 @@ struct osc_extent {
 	/* osc_object of this extent */
 	struct osc_object	*oe_obj;
 	/* refcount, removed from red-black tree if reaches zero. */
-	atomic_t		oe_refc;
+	struct kref		oe_refc;
 	/* busy if non-zero */
-	atomic_t		oe_users;
+	refcount_t		oe_users;
 	/* link list of osc_object's oo_{hp|urgent|locking}_exts. */
 	struct list_head	oe_link;
 	/* state of this extent */

[lustre-devel] [PATCH 10/29] lustre: osc_cache: avoid list_for_each_entry_safe when clearing list.

[NeilBrown]

When removing some items from a list, list_for_each_entry_safe() is a
good choice.
When removing all items, it is clearer to use a while loop
that repeatedly removes the first element, until there are
none left.  This makes it obvious that the list ends up
empty.

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

diff --git a/drivers/staging/lustre/lustre/osc/osc_cache.c b/drivers/staging/lustre/lustre/osc/osc_cache.c
index e65d917336b9..5cd3732101e7 100644
--- a/drivers/staging/lustre/lustre/osc/osc_cache.c
+++ b/drivers/staging/lustre/lustre/osc/osc_cache.c
@@ -869,7 +869,6 @@ int osc_extent_finish(const struct lu_env *env, struct osc_extent *ext,
 {
 	struct client_obd *cli = osc_cli(ext->oe_obj);
 	struct osc_async_page *oap;
-	struct osc_async_page *tmp;
 	int nr_pages = ext->oe_nr_pages;
 	int lost_grant = 0;
 	int blocksize = cli->cl_import->imp_obd->obd_osfs.os_bsize ? : 4096;
@@ -882,7 +881,9 @@ int osc_extent_finish(const struct lu_env *env, struct osc_extent *ext,
 	EASSERT(ergo(rc == 0, ext->oe_state == OES_RPC), ext);
 
 	osc_lru_add_batch(cli, &ext->oe_pages);
-	list_for_each_entry_safe(oap, tmp, &ext->oe_pages, oap_pending_item) {
+	while ((oap = list_first_entry_or_null(&ext->oe_pages,
+					       struct osc_async_page,
+					       oap_pending_item))) {
 		list_del_init(&oap->oap_rpc_item);
 		list_del_init(&oap->oap_pending_item);
 		if (last_off <= oap->oap_obj_off) {
@@ -1686,11 +1687,11 @@ static int osc_enter_cache(const struct lu_env *env, struct client_obd *cli,
 /* caller must hold loi_list_lock */
 void osc_wake_cache_waiters(struct client_obd *cli)
 {
-	struct list_head *l, *tmp;
 	struct osc_cache_waiter *ocw;
 
-	list_for_each_safe(l, tmp, &cli->cl_cache_waiters) {
-		ocw = list_entry(l, struct osc_cache_waiter, ocw_entry);
+	while ((ocw = list_first_entry_or_null(&cli->cl_cache_waiters,
+					       struct osc_cache_waiter,
+					       ocw_entry))) {
 		list_del_init(&ocw->ocw_entry);
 
 		ocw->ocw_rc = -EDQUOT;
@@ -2739,7 +2740,7 @@ int osc_queue_sync_pages(const struct lu_env *env, struct osc_object *obj,
 {
 	struct client_obd *cli = osc_cli(obj);
 	struct osc_extent *ext;
-	struct osc_async_page *oap, *tmp;
+	struct osc_async_page *oap;
 	int page_count = 0;
 	int mppr = cli->cl_max_pages_per_rpc;
 	bool can_merge = true;
@@ -2763,7 +2764,9 @@ int osc_queue_sync_pages(const struct lu_env *env, struct osc_object *obj,
 
 	ext = osc_extent_alloc(obj);
 	if (!ext) {
-		list_for_each_entry_safe(oap, tmp, list, oap_pending_item) {
+		while ((oap = list_first_entry_or_null(&oap->oap_pending_item,
+						       struct osc_async_page,
+						       oap_pending_item))) {
 			list_del_init(&oap->oap_pending_item);
 			osc_ap_completion(env, cli, oap, 0, -ENOMEM);
 		}
@@ -3093,11 +3096,12 @@ int osc_cache_writeback_range(const struct lu_env *env, struct osc_object *obj,
 
 	LASSERT(ergo(!discard, list_empty(&discard_list)));
 	if (!list_empty(&discard_list)) {
-		struct osc_extent *tmp;
 		int rc;
 
 		osc_list_maint(osc_cli(obj), obj);
-		list_for_each_entry_safe(ext, tmp, &discard_list, oe_link) {
+		while ((ext = list_first_entry_or_null(&discard_list,
+						       struct osc_extent,
+						       oe_link))) {
 			list_del_init(&ext->oe_link);
 			EASSERT(ext->oe_state == OES_LOCKING, ext);
 

[lustre-devel] [PATCH 10/29] lustre: osc_cache: avoid list_for_each_entry_safe when clearing list.

[Andreas Dilger]

On Jan 8, 2019, at 23:24, NeilBrown <neilb@suse.com> wrote:
> 
> When removing some items from a list, list_for_each_entry_safe() is a
> good choice.
> When removing all items, it is clearer to use a while loop
> that repeatedly removes the first element, until there are
> none left.  This makes it obvious that the list ends up
> empty.
> 
> Signed-off-by: NeilBrown <neilb@suse.com>

I've never heard of list_first_entry_or_null() before?  Looks like
it was added in 3.10, but definitely seems useful.  There are a
bunch of places that iterate lists this way while deleteing entries
that could be similarly improved.

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

> ---
> drivers/staging/lustre/lustre/osc/osc_cache.c |   22 +++++++++++++---------
> 1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/staging/lustre/lustre/osc/osc_cache.c b/drivers/staging/lustre/lustre/osc/osc_cache.c
> index e65d917336b9..5cd3732101e7 100644
> --- a/drivers/staging/lustre/lustre/osc/osc_cache.c
> +++ b/drivers/staging/lustre/lustre/osc/osc_cache.c
> @@ -869,7 +869,6 @@ int osc_extent_finish(const struct lu_env *env, struct osc_extent *ext,
> {
> 	struct client_obd *cli = osc_cli(ext->oe_obj);
> 	struct osc_async_page *oap;
> -	struct osc_async_page *tmp;
> 	int nr_pages = ext->oe_nr_pages;
> 	int lost_grant = 0;
> 	int blocksize = cli->cl_import->imp_obd->obd_osfs.os_bsize ? : 4096;
> @@ -882,7 +881,9 @@ int osc_extent_finish(const struct lu_env *env, struct osc_extent *ext,
> 	EASSERT(ergo(rc == 0, ext->oe_state == OES_RPC), ext);
> 
> 	osc_lru_add_batch(cli, &ext->oe_pages);
> -	list_for_each_entry_safe(oap, tmp, &ext->oe_pages, oap_pending_item) {
> +	while ((oap = list_first_entry_or_null(&ext->oe_pages,
> +					       struct osc_async_page,
> +					       oap_pending_item))) {
> 		list_del_init(&oap->oap_rpc_item);
> 		list_del_init(&oap->oap_pending_item);
> 		if (last_off <= oap->oap_obj_off) {
> @@ -1686,11 +1687,11 @@ static int osc_enter_cache(const struct lu_env *env, struct client_obd *cli,
> /* caller must hold loi_list_lock */
> void osc_wake_cache_waiters(struct client_obd *cli)
> {
> -	struct list_head *l, *tmp;
> 	struct osc_cache_waiter *ocw;
> 
> -	list_for_each_safe(l, tmp, &cli->cl_cache_waiters) {
> -		ocw = list_entry(l, struct osc_cache_waiter, ocw_entry);
> +	while ((ocw = list_first_entry_or_null(&cli->cl_cache_waiters,
> +					       struct osc_cache_waiter,
> +					       ocw_entry))) {
> 		list_del_init(&ocw->ocw_entry);
> 
> 		ocw->ocw_rc = -EDQUOT;
> @@ -2739,7 +2740,7 @@ int osc_queue_sync_pages(const struct lu_env *env, struct osc_object *obj,
> {
> 	struct client_obd *cli = osc_cli(obj);
> 	struct osc_extent *ext;
> -	struct osc_async_page *oap, *tmp;
> +	struct osc_async_page *oap;
> 	int page_count = 0;
> 	int mppr = cli->cl_max_pages_per_rpc;
> 	bool can_merge = true;
> @@ -2763,7 +2764,9 @@ int osc_queue_sync_pages(const struct lu_env *env, struct osc_object *obj,
> 
> 	ext = osc_extent_alloc(obj);
> 	if (!ext) {
> -		list_for_each_entry_safe(oap, tmp, list, oap_pending_item) {
> +		while ((oap = list_first_entry_or_null(&oap->oap_pending_item,
> +						       struct osc_async_page,
> +						       oap_pending_item))) {
> 			list_del_init(&oap->oap_pending_item);
> 			osc_ap_completion(env, cli, oap, 0, -ENOMEM);
> 		}
> @@ -3093,11 +3096,12 @@ int osc_cache_writeback_range(const struct lu_env *env, struct osc_object *obj,
> 
> 	LASSERT(ergo(!discard, list_empty(&discard_list)));
> 	if (!list_empty(&discard_list)) {
> -		struct osc_extent *tmp;
> 		int rc;
> 
> 		osc_list_maint(osc_cli(obj), obj);
> -		list_for_each_entry_safe(ext, tmp, &discard_list, oe_link) {
> +		while ((ext = list_first_entry_or_null(&discard_list,
> +						       struct osc_extent,
> +						       oe_link))) {
> 			list_del_init(&ext->oe_link);
> 			EASSERT(ext->oe_state == OES_LOCKING, ext);
> 
> 
> 

Cheers, Andreas
---
Andreas Dilger
Principal Lustre Architect
Whamcloud

[lustre-devel] [PATCH 02/29] lustre: osc_cache: use assert_spin_locked()

[NeilBrown]

assert_spin_locked() is preferred to
spin_is_locked() for affirming that a
spinlock is locked.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 drivers/staging/lustre/lustre/osc/osc_cache.c      |   29 +++++++++-----------
 .../staging/lustre/lustre/osc/osc_cl_internal.h    |   15 +---------
 2 files changed, 15 insertions(+), 29 deletions(-)

diff --git a/drivers/staging/lustre/lustre/osc/osc_cache.c b/drivers/staging/lustre/lustre/osc/osc_cache.c
index fbf16547003d..1ce9f673f1bf 100644
--- a/drivers/staging/lustre/lustre/osc/osc_cache.c
+++ b/drivers/staging/lustre/lustre/osc/osc_cache.c
@@ -181,10 +181,7 @@ static int osc_extent_sanity_check0(struct osc_extent *ext,
 	size_t page_count;
 	int rc = 0;
 
-	if (!osc_object_is_locked(obj)) {
-		rc = 9;
-		goto out;
-	}
+	assert_osc_object_is_locked(obj);
 
 	if (ext->oe_state >= OES_STATE_MAX) {
 		rc = 10;
@@ -324,7 +321,7 @@ static int osc_extent_is_overlapped(struct osc_object *obj,
 {
 	struct osc_extent *tmp;
 
-	LASSERT(osc_object_is_locked(obj));
+	assert_osc_object_is_locked(obj);
 
 	if (!extent_debug)
 		return 0;
@@ -341,7 +338,7 @@ static int osc_extent_is_overlapped(struct osc_object *obj,
 
 static void osc_extent_state_set(struct osc_extent *ext, int state)
 {
-	LASSERT(osc_object_is_locked(ext->oe_obj));
+	assert_osc_object_is_locked(ext->oe_obj);
 	LASSERT(state >= OES_INV && state < OES_STATE_MAX);
 
 	/* Never try to sanity check a state changing extent :-) */
@@ -414,7 +411,7 @@ static void osc_extent_put(const struct lu_env *env, struct osc_extent *ext)
 static void osc_extent_put_trust(struct osc_extent *ext)
 {
 	LASSERT(atomic_read(&ext->oe_refc) > 1);
-	LASSERT(osc_object_is_locked(ext->oe_obj));
+	assert_osc_object_is_locked(ext->oe_obj);
 	atomic_dec(&ext->oe_refc);
 }
 
@@ -428,7 +425,7 @@ static struct osc_extent *osc_extent_search(struct osc_object *obj,
 	struct rb_node *n = obj->oo_root.rb_node;
 	struct osc_extent *tmp, *p = NULL;
 
-	LASSERT(osc_object_is_locked(obj));
+	assert_osc_object_is_locked(obj);
 	while (n) {
 		tmp = rb_extent(n);
 		if (index < tmp->oe_start) {
@@ -467,7 +464,7 @@ static void osc_extent_insert(struct osc_object *obj, struct osc_extent *ext)
 
 	LASSERT(RB_EMPTY_NODE(&ext->oe_node));
 	LASSERT(ext->oe_obj == obj);
-	LASSERT(osc_object_is_locked(obj));
+	assert_osc_object_is_locked(obj);
 	while (*n) {
 		tmp = rb_extent(*n);
 		parent = *n;
@@ -489,7 +486,7 @@ static void osc_extent_erase(struct osc_extent *ext)
 {
 	struct osc_object *obj = ext->oe_obj;
 
-	LASSERT(osc_object_is_locked(obj));
+	assert_osc_object_is_locked(obj);
 	if (!RB_EMPTY_NODE(&ext->oe_node)) {
 		rb_erase(&ext->oe_node, &obj->oo_root);
 		RB_CLEAR_NODE(&ext->oe_node);
@@ -502,7 +499,7 @@ static struct osc_extent *osc_extent_hold(struct osc_extent *ext)
 {
 	struct osc_object *obj = ext->oe_obj;
 
-	LASSERT(osc_object_is_locked(obj));
+	assert_osc_object_is_locked(obj);
 	LASSERT(ext->oe_state == OES_ACTIVE || ext->oe_state == OES_CACHE);
 	if (ext->oe_state == OES_CACHE) {
 		osc_extent_state_set(ext, OES_ACTIVE);
@@ -515,7 +512,7 @@ static struct osc_extent *osc_extent_hold(struct osc_extent *ext)
 
 static void __osc_extent_remove(struct osc_extent *ext)
 {
-	LASSERT(osc_object_is_locked(ext->oe_obj));
+	assert_osc_object_is_locked(ext->oe_obj);
 	LASSERT(list_empty(&ext->oe_pages));
 	osc_extent_erase(ext);
 	list_del_init(&ext->oe_link);
@@ -546,7 +543,7 @@ static int osc_extent_merge(const struct lu_env *env, struct osc_extent *cur,
 	int ppc_bits;
 
 	LASSERT(cur->oe_state == OES_CACHE);
-	LASSERT(osc_object_is_locked(obj));
+	assert_osc_object_is_locked(obj);
 	if (!victim)
 		return -EINVAL;
 
@@ -2079,7 +2076,7 @@ static unsigned int get_write_extents(struct osc_object *obj,
 		.erd_max_extents = 256,
 	};
 
-	LASSERT(osc_object_is_locked(obj));
+	assert_osc_object_is_locked(obj);
 	while (!list_empty(&obj->oo_hp_exts)) {
 		ext = list_entry(obj->oo_hp_exts.next, struct osc_extent,
 				 oe_link);
@@ -2146,7 +2143,7 @@ osc_send_write_rpc(const struct lu_env *env, struct client_obd *cli,
 	int srvlock = 0;
 	int rc = 0;
 
-	LASSERT(osc_object_is_locked(osc));
+	assert_osc_object_is_locked(osc);
 
 	page_count = get_write_extents(osc, &rpclist);
 	LASSERT(equi(page_count == 0, list_empty(&rpclist)));
@@ -2224,7 +2221,7 @@ osc_send_read_rpc(const struct lu_env *env, struct client_obd *cli,
 	};
 	int rc = 0;
 
-	LASSERT(osc_object_is_locked(osc));
+	assert_osc_object_is_locked(osc);
 	list_for_each_entry_safe(ext, next, &osc->oo_reading_exts, oe_link) {
 		EASSERT(ext->oe_state == OES_LOCK_DONE, ext);
 		if (!try_to_add_extent_for_io(cli, ext, &data))
diff --git a/drivers/staging/lustre/lustre/osc/osc_cl_internal.h b/drivers/staging/lustre/lustre/osc/osc_cl_internal.h
index b78deef3963a..aa1b753fc88d 100644
--- a/drivers/staging/lustre/lustre/osc/osc_cl_internal.h
+++ b/drivers/staging/lustre/lustre/osc/osc_cl_internal.h
@@ -183,19 +183,8 @@ static inline void osc_object_unlock(struct osc_object *obj)
 	spin_unlock(&obj->oo_lock);
 }
 
-static inline int osc_object_is_locked(struct osc_object *obj)
-{
-#if defined(CONFIG_SMP) || defined(CONFIG_DEBUG_SPINLOCK)
-	return spin_is_locked(&obj->oo_lock);
-#else
-	/*
-	 * It is not perfect to return true all the time.
-	 * But since this function is only used for assertion
-	 * and checking, it seems OK.
-	 */
-	return 1;
-#endif
-}
+#define assert_osc_object_is_locked(obj)	\
+	assert_spin_locked(&obj->oo_lock)
 
 /*
  * Lock "micro-states" for osc layer.

[lustre-devel] [PATCH 02/29] lustre: osc_cache: use assert_spin_locked()

[Andreas Dilger]

On Jan 8, 2019, at 23:24, NeilBrown <neilb@suse.com> wrote:
> 
> assert_spin_locked() is preferred to
> spin_is_locked() for affirming that a
> spinlock is locked.
> 
> Signed-off-by: NeilBrown <neilb@suse.com>

We used to have an LASSERT_SPIN_LOCKED() macro, but it was removed a few
years ago.  It is nice to get better checking in the kernel.

One question inline below:

> ---
> drivers/staging/lustre/lustre/osc/osc_cache.c      |   29 +++++++++-----------
> .../staging/lustre/lustre/osc/osc_cl_internal.h    |   15 +---------
> 2 files changed, 15 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/staging/lustre/lustre/osc/osc_cache.c b/drivers/staging/lustre/lustre/osc/osc_cache.c
> index fbf16547003d..1ce9f673f1bf 100644
> --- a/drivers/staging/lustre/lustre/osc/osc_cache.c
> +++ b/drivers/staging/lustre/lustre/osc/osc_cache.c
> @@ -181,10 +181,7 @@ static int osc_extent_sanity_check0(struct osc_extent *ext,
> 	size_t page_count;
> 	int rc = 0;
> 
> -	if (!osc_object_is_locked(obj)) {
> -		rc = 9;
> -		goto out;
> -	}
> +	assert_osc_object_is_locked(obj);

Is this actually a fatal error?  It looks like if the object is not
locked then the checking isn't consistent and could just be skipped?

There is a macro that lends credence to this:

#define sanity_check_nolock(ext) \
        osc_extent_sanity_check0(ext, __func__, __LINE__)

#define sanity_check(ext) ({                                   \
        int __res;                                             \
        osc_object_lock((ext)->oe_obj);                        \
        __res = sanity_check_nolock(ext);                      \
        osc_object_unlock((ext)->oe_obj);                      \
        __res;                                                 \
})

However, reading deeper into the code sanity_check_nolock() looks
like is only ever called when the object is already locked, so
indeed it does seem like a valid change that should be described
in the commit message like:

    __osc_extent_sanity_check() is only ever called with obj
    already locked, so change the check into an assertion.

It might also be an improvement to rename sanity_check{,_nolock}()
to osc_extent_sanity_check() and osc_extent_sanity_check_nolock(),
and use __osc_extent_sanity_check() instead of ...0() (which is not
standard)?  I was going to suggest making sanity_check() an inline
function, but that would break the __func__ and __LINE__ expansion
and isn't onerous.

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

> 
> 	if (ext->oe_state >= OES_STATE_MAX) {
> 		rc = 10;
> @@ -324,7 +321,7 @@ static int osc_extent_is_overlapped(struct osc_object *obj,
> {
> 	struct osc_extent *tmp;
> 
> -	LASSERT(osc_object_is_locked(obj));
> +	assert_osc_object_is_locked(obj);
> 
> 	if (!extent_debug)
> 		return 0;
> @@ -341,7 +338,7 @@ static int osc_extent_is_overlapped(struct osc_object *obj,
> 
> static void osc_extent_state_set(struct osc_extent *ext, int state)
> {
> -	LASSERT(osc_object_is_locked(ext->oe_obj));
> +	assert_osc_object_is_locked(ext->oe_obj);
> 	LASSERT(state >= OES_INV && state < OES_STATE_MAX);
> 
> 	/* Never try to sanity check a state changing extent :-) */
> @@ -414,7 +411,7 @@ static void osc_extent_put(const struct lu_env *env, struct osc_extent *ext)
> static void osc_extent_put_trust(struct osc_extent *ext)
> {
> 	LASSERT(atomic_read(&ext->oe_refc) > 1);
> -	LASSERT(osc_object_is_locked(ext->oe_obj));
> +	assert_osc_object_is_locked(ext->oe_obj);
> 	atomic_dec(&ext->oe_refc);
> }
> 
> @@ -428,7 +425,7 @@ static struct osc_extent *osc_extent_search(struct osc_object *obj,
> 	struct rb_node *n = obj->oo_root.rb_node;
> 	struct osc_extent *tmp, *p = NULL;
> 
> -	LASSERT(osc_object_is_locked(obj));
> +	assert_osc_object_is_locked(obj);
> 	while (n) {
> 		tmp = rb_extent(n);
> 		if (index < tmp->oe_start) {
> @@ -467,7 +464,7 @@ static void osc_extent_insert(struct osc_object *obj, struct osc_extent *ext)
> 
> 	LASSERT(RB_EMPTY_NODE(&ext->oe_node));
> 	LASSERT(ext->oe_obj == obj);
> -	LASSERT(osc_object_is_locked(obj));
> +	assert_osc_object_is_locked(obj);
> 	while (*n) {
> 		tmp = rb_extent(*n);
> 		parent = *n;
> @@ -489,7 +486,7 @@ static void osc_extent_erase(struct osc_extent *ext)
> {
> 	struct osc_object *obj = ext->oe_obj;
> 
> -	LASSERT(osc_object_is_locked(obj));
> +	assert_osc_object_is_locked(obj);
> 	if (!RB_EMPTY_NODE(&ext->oe_node)) {
> 		rb_erase(&ext->oe_node, &obj->oo_root);
> 		RB_CLEAR_NODE(&ext->oe_node);
> @@ -502,7 +499,7 @@ static struct osc_extent *osc_extent_hold(struct osc_extent *ext)
> {
> 	struct osc_object *obj = ext->oe_obj;
> 
> -	LASSERT(osc_object_is_locked(obj));
> +	assert_osc_object_is_locked(obj);
> 	LASSERT(ext->oe_state == OES_ACTIVE || ext->oe_state == OES_CACHE);
> 	if (ext->oe_state == OES_CACHE) {
> 		osc_extent_state_set(ext, OES_ACTIVE);
> @@ -515,7 +512,7 @@ static struct osc_extent *osc_extent_hold(struct osc_extent *ext)
> 
> static void __osc_extent_remove(struct osc_extent *ext)
> {
> -	LASSERT(osc_object_is_locked(ext->oe_obj));
> +	assert_osc_object_is_locked(ext->oe_obj);
> 	LASSERT(list_empty(&ext->oe_pages));
> 	osc_extent_erase(ext);
> 	list_del_init(&ext->oe_link);
> @@ -546,7 +543,7 @@ static int osc_extent_merge(const struct lu_env *env, struct osc_extent *cur,
> 	int ppc_bits;
> 
> 	LASSERT(cur->oe_state == OES_CACHE);
> -	LASSERT(osc_object_is_locked(obj));
> +	assert_osc_object_is_locked(obj);
> 	if (!victim)
> 		return -EINVAL;
> 
> @@ -2079,7 +2076,7 @@ static unsigned int get_write_extents(struct osc_object *obj,
> 		.erd_max_extents = 256,
> 	};
> 
> -	LASSERT(osc_object_is_locked(obj));
> +	assert_osc_object_is_locked(obj);
> 	while (!list_empty(&obj->oo_hp_exts)) {
> 		ext = list_entry(obj->oo_hp_exts.next, struct osc_extent,
> 				 oe_link);
> @@ -2146,7 +2143,7 @@ osc_send_write_rpc(const struct lu_env *env, struct client_obd *cli,
> 	int srvlock = 0;
> 	int rc = 0;
> 
> -	LASSERT(osc_object_is_locked(osc));
> +	assert_osc_object_is_locked(osc);
> 
> 	page_count = get_write_extents(osc, &rpclist);
> 	LASSERT(equi(page_count == 0, list_empty(&rpclist)));
> @@ -2224,7 +2221,7 @@ osc_send_read_rpc(const struct lu_env *env, struct client_obd *cli,
> 	};
> 	int rc = 0;
> 
> -	LASSERT(osc_object_is_locked(osc));
> +	assert_osc_object_is_locked(osc);
> 	list_for_each_entry_safe(ext, next, &osc->oo_reading_exts, oe_link) {
> 		EASSERT(ext->oe_state == OES_LOCK_DONE, ext);
> 		if (!try_to_add_extent_for_io(cli, ext, &data))
> diff --git a/drivers/staging/lustre/lustre/osc/osc_cl_internal.h b/drivers/staging/lustre/lustre/osc/osc_cl_internal.h
> index b78deef3963a..aa1b753fc88d 100644
> --- a/drivers/staging/lustre/lustre/osc/osc_cl_internal.h
> +++ b/drivers/staging/lustre/lustre/osc/osc_cl_internal.h
> @@ -183,19 +183,8 @@ static inline void osc_object_unlock(struct osc_object *obj)
> 	spin_unlock(&obj->oo_lock);
> }
> 
> -static inline int osc_object_is_locked(struct osc_object *obj)
> -{
> -#if defined(CONFIG_SMP) || defined(CONFIG_DEBUG_SPINLOCK)
> -	return spin_is_locked(&obj->oo_lock);
> -#else
> -	/*
> -	 * It is not perfect to return true all the time.
> -	 * But since this function is only used for assertion
> -	 * and checking, it seems OK.
> -	 */
> -	return 1;
> -#endif
> -}
> +#define assert_osc_object_is_locked(obj)	\
> +	assert_spin_locked(&obj->oo_lock)
> 
> /*
>  * Lock "micro-states" for osc layer.
> 
> 

Cheers, Andreas
---
Andreas Dilger
CTO Whamcloud

[lustre-devel] [PATCH 02/29] lustre: osc_cache: use assert_spin_locked()

[NeilBrown]

On Thu, Jan 10 2019, Andreas Dilger wrote:

> On Jan 8, 2019, at 23:24, NeilBrown <neilb@suse.com> wrote:
>> 
>> assert_spin_locked() is preferred to
>> spin_is_locked() for affirming that a
>> spinlock is locked.
>> 
>> Signed-off-by: NeilBrown <neilb@suse.com>
>
> We used to have an LASSERT_SPIN_LOCKED() macro, but it was removed a few
> years ago.  It is nice to get better checking in the kernel.
>
> One question inline below:
>
>> ---
>> drivers/staging/lustre/lustre/osc/osc_cache.c      |   29 +++++++++-----------
>> .../staging/lustre/lustre/osc/osc_cl_internal.h    |   15 +---------
>> 2 files changed, 15 insertions(+), 29 deletions(-)
>> 
>> diff --git a/drivers/staging/lustre/lustre/osc/osc_cache.c b/drivers/staging/lustre/lustre/osc/osc_cache.c
>> index fbf16547003d..1ce9f673f1bf 100644
>> --- a/drivers/staging/lustre/lustre/osc/osc_cache.c
>> +++ b/drivers/staging/lustre/lustre/osc/osc_cache.c
>> @@ -181,10 +181,7 @@ static int osc_extent_sanity_check0(struct osc_extent *ext,
>> 	size_t page_count;
>> 	int rc = 0;
>> 
>> -	if (!osc_object_is_locked(obj)) {
>> -		rc = 9;
>> -		goto out;
>> -	}
>> +	assert_osc_object_is_locked(obj);
>
> Is this actually a fatal error?  It looks like if the object is not
> locked then the checking isn't consistent and could just be skipped?

The assert is fatal (-> BUG_ON()).

>
> There is a macro that lends credence to this:
>
> #define sanity_check_nolock(ext) \
>         osc_extent_sanity_check0(ext, __func__, __LINE__)
>
> #define sanity_check(ext) ({                                   \
>         int __res;                                             \
>         osc_object_lock((ext)->oe_obj);                        \
>         __res = sanity_check_nolock(ext);                      \
>         osc_object_unlock((ext)->oe_obj);                      \
>         __res;                                                 \
> })
>
> However, reading deeper into the code sanity_check_nolock() looks
> like is only ever called when the object is already locked, so
> indeed it does seem like a valid change that should be described
> in the commit message like:

Agreed.

>
>     __osc_extent_sanity_check() is only ever called with obj
>     already locked, so change the check into an assertion.

Good suggestion - I've added this text.

>
> It might also be an improvement to rename sanity_check{,_nolock}()
> to osc_extent_sanity_check() and osc_extent_sanity_check_nolock(),
> and use __osc_extent_sanity_check() instead of ...0() (which is not
> standard)?  I was going to suggest making sanity_check() an inline
> function, but that would break the __func__ and __LINE__ expansion
> and isn't onerous.

That renaming makes sense - I'll add it as another patch.

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

Thanks,
NeilBrown
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <http://lists.lustre.org/pipermail/lustre-devel-lustre.org/attachments/20190110/eb293c70/attachment.sig>

[lustre-devel] [PATCH 07/29] lustre: osc: convert a while loop to for

[NeilBrown]

This loop uses 'continue' in several places,
and each one is proceeded by
   ext = next_extent(ext)
which also appears at the end.
This is exactly the pattern that a 'for' loop
simplifies.  So change to a for loop.

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

diff --git a/drivers/staging/lustre/lustre/osc/osc_cache.c b/drivers/staging/lustre/lustre/osc/osc_cache.c
index dd3c87124aa5..eb8de1503386 100644
--- a/drivers/staging/lustre/lustre/osc/osc_cache.c
+++ b/drivers/staging/lustre/lustre/osc/osc_cache.c
@@ -737,7 +737,7 @@ static struct osc_extent *osc_extent_find(const struct lu_env *env,
 	ext = osc_extent_search(obj, cur->oe_start);
 	if (!ext)
 		ext = first_extent(obj);
-	while (ext) {
+	for (; ext; ext = next_extent(ext)) {
 		pgoff_t ext_chk_start = ext->oe_start >> ppc_bits;
 		pgoff_t ext_chk_end = ext->oe_end >> ppc_bits;
 
@@ -750,15 +750,12 @@ static struct osc_extent *osc_extent_find(const struct lu_env *env,
 			EASSERTF(!overlapped(ext, cur), ext,
 				 EXTSTR "\n", EXTPARA(cur));
 
-			ext = next_extent(ext);
 			continue;
 		}
 
 		/* discontiguous chunks? */
-		if (chunk + 1 < ext_chk_start) {
-			ext = next_extent(ext);
+		if (chunk + 1 < ext_chk_start)
 			continue;
-		}
 
 		/* ok, from now on, ext and cur have these attrs:
 		 * 1. covered by the same lock
@@ -786,33 +783,27 @@ static struct osc_extent *osc_extent_find(const struct lu_env *env,
 		}
 
 		/* non-overlapped extent */
-		if (ext->oe_state != OES_CACHE || ext->oe_fsync_wait) {
+		if (ext->oe_state != OES_CACHE || ext->oe_fsync_wait)
 			/* we can't do anything for a non OES_CACHE extent, or
 			 * if there is someone waiting for this extent to be
 			 * flushed, try next one.
 			 */
-			ext = next_extent(ext);
 			continue;
-		}
 
 		/* check if they belong to the same rpc slot before trying to
 		 * merge. the extents are not overlapped and contiguous at
 		 * chunk level to get here.
 		 */
-		if (ext->oe_max_end != max_end) {
+		if (ext->oe_max_end != max_end)
 			/* if they don't belong to the same RPC slot or
 			 * max_pages_per_rpc has ever changed, do not merge.
 			 */
-			ext = next_extent(ext);
 			continue;
-		}
 
 		/* check whether maximum extent size will be hit */
 		if ((ext_chk_end - ext_chk_start + 1) << ppc_bits >
-		    cli->cl_max_extent_pages) {
-			ext = next_extent(ext);
+		    cli->cl_max_extent_pages)
 			continue;
-		}
 
 		/* it's required that an extent must be contiguous at chunk
 		 * level so that we know the whole extent is covered by grant
@@ -851,8 +842,6 @@ static struct osc_extent *osc_extent_find(const struct lu_env *env,
 		}
 		if (found)
 			break;
-
-		ext = next_extent(ext);
 	}
 
 	osc_extent_tree_dump(D_CACHE, obj);

[lustre-devel] [PATCH 07/29] lustre: osc: convert a while loop to for

[Andreas Dilger]

On Jan 8, 2019, at 23:24, NeilBrown <neilb@suse.com> wrote:
> 
> This loop uses 'continue' in several places,
> and each one is proceeded by
>   ext = next_extent(ext)
> which also appears at the end.
> This is exactly the pattern that a 'for' loop
> simplifies.  So change to a for loop.
> 
> Signed-off-by: NeilBrown <neilb@suse.com>

Much nicer and less error prone,

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

> ---
> drivers/staging/lustre/lustre/osc/osc_cache.c |   21 +++++----------------
> 1 file changed, 5 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/staging/lustre/lustre/osc/osc_cache.c b/drivers/staging/lustre/lustre/osc/osc_cache.c
> index dd3c87124aa5..eb8de1503386 100644
> --- a/drivers/staging/lustre/lustre/osc/osc_cache.c
> +++ b/drivers/staging/lustre/lustre/osc/osc_cache.c
> @@ -737,7 +737,7 @@ static struct osc_extent *osc_extent_find(const struct lu_env *env,
> 	ext = osc_extent_search(obj, cur->oe_start);
> 	if (!ext)
> 		ext = first_extent(obj);
> -	while (ext) {
> +	for (; ext; ext = next_extent(ext)) {
> 		pgoff_t ext_chk_start = ext->oe_start >> ppc_bits;
> 		pgoff_t ext_chk_end = ext->oe_end >> ppc_bits;
> 
> @@ -750,15 +750,12 @@ static struct osc_extent *osc_extent_find(const struct lu_env *env,
> 			EASSERTF(!overlapped(ext, cur), ext,
> 				 EXTSTR "\n", EXTPARA(cur));
> 
> -			ext = next_extent(ext);
> 			continue;
> 		}
> 
> 		/* discontiguous chunks? */
> -		if (chunk + 1 < ext_chk_start) {
> -			ext = next_extent(ext);
> +		if (chunk + 1 < ext_chk_start)
> 			continue;
> -		}
> 
> 		/* ok, from now on, ext and cur have these attrs:
> 		 * 1. covered by the same lock
> @@ -786,33 +783,27 @@ static struct osc_extent *osc_extent_find(const struct lu_env *env,
> 		}
> 
> 		/* non-overlapped extent */
> -		if (ext->oe_state != OES_CACHE || ext->oe_fsync_wait) {
> +		if (ext->oe_state != OES_CACHE || ext->oe_fsync_wait)
> 			/* we can't do anything for a non OES_CACHE extent, or
> 			 * if there is someone waiting for this extent to be
> 			 * flushed, try next one.
> 			 */
> -			ext = next_extent(ext);
> 			continue;
> -		}
> 
> 		/* check if they belong to the same rpc slot before trying to
> 		 * merge. the extents are not overlapped and contiguous at
> 		 * chunk level to get here.
> 		 */
> -		if (ext->oe_max_end != max_end) {
> +		if (ext->oe_max_end != max_end)
> 			/* if they don't belong to the same RPC slot or
> 			 * max_pages_per_rpc has ever changed, do not merge.
> 			 */
> -			ext = next_extent(ext);
> 			continue;
> -		}
> 
> 		/* check whether maximum extent size will be hit */
> 		if ((ext_chk_end - ext_chk_start + 1) << ppc_bits >
> -		    cli->cl_max_extent_pages) {
> -			ext = next_extent(ext);
> +		    cli->cl_max_extent_pages)
> 			continue;
> -		}
> 
> 		/* it's required that an extent must be contiguous at chunk
> 		 * level so that we know the whole extent is covered by grant
> @@ -851,8 +842,6 @@ static struct osc_extent *osc_extent_find(const struct lu_env *env,
> 		}
> 		if (found)
> 			break;
> -
> -		ext = next_extent(ext);
> 	}
> 
> 	osc_extent_tree_dump(D_CACHE, obj);
> 
> 

Cheers, Andreas
---
Andreas Dilger
CTO Whamcloud

[lustre-devel] [PATCH 11/29] lustre: osc_cache: simplify osc_wake_cache_waiters()

[NeilBrown]

osc_wake_cache_waiters() performs a test, then possibly
calls osc_enter_cache_try() which performs the same test.
We don't benefit from this duplication except that it allows
an extra debug message.  I'm not certain that message
is worth the complexity.

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

diff --git a/drivers/staging/lustre/lustre/osc/osc_cache.c b/drivers/staging/lustre/lustre/osc/osc_cache.c
index 5cd3732101e7..0c78b95e45a3 100644
--- a/drivers/staging/lustre/lustre/osc/osc_cache.c
+++ b/drivers/staging/lustre/lustre/osc/osc_cache.c
@@ -1694,20 +1694,11 @@ void osc_wake_cache_waiters(struct client_obd *cli)
 					       ocw_entry))) {
 		list_del_init(&ocw->ocw_entry);
 
-		ocw->ocw_rc = -EDQUOT;
-		/* we can't dirty more */
-		if ((cli->cl_dirty_pages > cli->cl_dirty_max_pages) ||
-		    (atomic_long_read(&obd_dirty_pages) + 1 >
-		     obd_max_dirty_pages)) {
-			CDEBUG(D_CACHE, "no dirty room: dirty: %ld osc max %ld, sys max %ld\n",
-			       cli->cl_dirty_pages, cli->cl_dirty_max_pages,
-			       obd_max_dirty_pages);
-			goto wakeup;
-		}
-
 		if (osc_enter_cache_try(cli, ocw->ocw_oap, ocw->ocw_grant, 0))
 			ocw->ocw_rc = 0;
-wakeup:
+		else
+			ocw->ocw_rc = -EDQUOT;
+
 		CDEBUG(D_CACHE, "wake up %p for oap %p, avail grant %ld, %d\n",
 		       ocw, ocw->ocw_oap, cli->cl_avail_grant, ocw->ocw_rc);
 

[lustre-devel] [PATCH 04/29] lustre: osc: simplify list manipulation

[NeilBrown]

when A is empty,
 list_move_tail(&A, &B);
is identical to
 list_add_tail(&A, &B);

so always use list_move_tail() - it is easier to understand.

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

diff --git a/drivers/staging/lustre/lustre/osc/osc_cache.c b/drivers/staging/lustre/lustre/osc/osc_cache.c
index 00056dffceb9..6771675dd520 100644
--- a/drivers/staging/lustre/lustre/osc/osc_cache.c
+++ b/drivers/staging/lustre/lustre/osc/osc_cache.c
@@ -2885,10 +2885,7 @@ int osc_cache_truncate_start(const struct lu_env *env, struct osc_object *obj,
 		}
 		/* This extent could be on the full extents list, that's OK */
 		EASSERT(!ext->oe_hp && !ext->oe_urgent, ext);
-		if (!list_empty(&ext->oe_link))
-			list_move_tail(&ext->oe_link, &list);
-		else
-			list_add_tail(&ext->oe_link, &list);
+		list_move_tail(&ext->oe_link, &list);
 
 		ext = next_extent(ext);
 	}

[lustre-devel] [PATCH 04/29] lustre: osc: simplify list manipulation

[Andreas Dilger]

> On Jan 8, 2019, at 23:24, NeilBrown <neilb@suse.com> wrote:
> 
> when A is empty,
> list_move_tail(&A, &B);
> is identical to
> list_add_tail(&A, &B);
> 
> so always use list_move_tail() - it is easier to understand.
> 
> Signed-off-by: NeilBrown <neilb@suse.com>

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

> ---
> drivers/staging/lustre/lustre/osc/osc_cache.c |    5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/lustre/lustre/osc/osc_cache.c b/drivers/staging/lustre/lustre/osc/osc_cache.c
> index 00056dffceb9..6771675dd520 100644
> --- a/drivers/staging/lustre/lustre/osc/osc_cache.c
> +++ b/drivers/staging/lustre/lustre/osc/osc_cache.c
> @@ -2885,10 +2885,7 @@ int osc_cache_truncate_start(const struct lu_env *env, struct osc_object *obj,
> 		}
> 		/* This extent could be on the full extents list, that's OK */
> 		EASSERT(!ext->oe_hp && !ext->oe_urgent, ext);
> -		if (!list_empty(&ext->oe_link))
> -			list_move_tail(&ext->oe_link, &list);
> -		else
> -			list_add_tail(&ext->oe_link, &list);
> +		list_move_tail(&ext->oe_link, &list);
> 
> 		ext = next_extent(ext);
> 	}
> 
> 

Cheers, Andreas
---
Andreas Dilger
CTO Whamcloud

[lustre-devel] [PATCH 12/29] lustre: osc_cache: avoid confusing variable reuse.

[NeilBrown]

In osc_extent_wait(), the 'rc' variable use used for
three very different purposes.  This makes the code
a bit harder to read.
For one of the purposes, introduce a boolean 'need_release'.
For another, use a conditional expression instead.

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

diff --git a/drivers/staging/lustre/lustre/osc/osc_cache.c b/drivers/staging/lustre/lustre/osc/osc_cache.c
index 0c78b95e45a3..5a295799f177 100644
--- a/drivers/staging/lustre/lustre/osc/osc_cache.c
+++ b/drivers/staging/lustre/lustre/osc/osc_cache.c
@@ -929,7 +929,8 @@ static int osc_extent_wait(const struct lu_env *env, struct osc_extent *ext,
 			   enum osc_extent_state state)
 {
 	struct osc_object *obj = ext->oe_obj;
-	int rc = 0;
+	bool need_release = false;
+	int rc;
 
 	osc_object_lock(obj);
 	LASSERT(sanity_check_nolock(ext) == 0);
@@ -943,11 +944,11 @@ static int osc_extent_wait(const struct lu_env *env, struct osc_extent *ext,
 		} else if (ext->oe_state == OES_CACHE) {
 			ext->oe_urgent = 1;
 			osc_extent_hold(ext);
-			rc = 1;
+			need_release = true;
 		}
 	}
 	osc_object_unlock(obj);
-	if (rc == 1)
+	if (need_release)
 		osc_extent_release(env, ext);
 
 	/* wait for the extent until its state becomes @state */
@@ -963,11 +964,7 @@ static int osc_extent_wait(const struct lu_env *env, struct osc_extent *ext,
 				smp_load_acquire(&ext->oe_state) == state);
 
 	}
-	if (ext->oe_rc < 0)
-		rc = ext->oe_rc;
-	else
-		rc = 0;
-	return rc;
+	return ext->oe_rc < 0 ? ext->oe_rc : 0;
 }
 
 /**

[lustre-devel] [PATCH 08/29] lustre: osc: simplify osc_extent_find()

[NeilBrown]

osc_extent_find() contains some code with the same functionality as
osc_extent_merge().  So replace that code with a call to
osc_extent_merge().

This requires that we:
 - set cur->oe_grants earlier, as osc_extent_merge() needs that
 - take an extra temporary ref to cur, as osc_extent_merge()
   will drop the ref we have on success.

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

diff --git a/drivers/staging/lustre/lustre/osc/osc_cache.c b/drivers/staging/lustre/lustre/osc/osc_cache.c
index eb8de1503386..848e440ae2a9 100644
--- a/drivers/staging/lustre/lustre/osc/osc_cache.c
+++ b/drivers/staging/lustre/lustre/osc/osc_cache.c
@@ -718,7 +718,8 @@ static struct osc_extent *osc_extent_find(const struct lu_env *env,
 		cur->oe_start = descr->cld_start;
 	if (cur->oe_end > max_end)
 		cur->oe_end = max_end;
-	cur->oe_grants = 0;
+	LASSERT(*grants >= chunksize);
+	cur->oe_grants = chunksize;
 	cur->oe_mppr = max_pages;
 	if (olck->ols_dlmlock) {
 		LASSERT(olck->ols_hold);
@@ -790,58 +791,31 @@ static struct osc_extent *osc_extent_find(const struct lu_env *env,
 			 */
 			continue;
 
-		/* check if they belong to the same rpc slot before trying to
-		 * merge. the extents are not overlapped and contiguous at
-		 * chunk level to get here.
-		 */
-		if (ext->oe_max_end != max_end)
-			/* if they don't belong to the same RPC slot or
-			 * max_pages_per_rpc has ever changed, do not merge.
-			 */
-			continue;
-
-		/* check whether maximum extent size will be hit */
-		if ((ext_chk_end - ext_chk_start + 1) << ppc_bits >
-		    cli->cl_max_extent_pages)
-			continue;
-
 		/* it's required that an extent must be contiguous at chunk
 		 * level so that we know the whole extent is covered by grant
 		 * (the pages in the extent are NOT required to be contiguous).
 		 * Otherwise, it will be too much difficult to know which
 		 * chunks have grants allocated.
 		 */
-
-		/* try to do front merge - extend ext's start */
-		if (chunk + 1 == ext_chk_start) {
-			/* ext must be chunk size aligned */
-			EASSERT((ext->oe_start & ~chunk_mask) == 0, ext);
-
-			/* pull ext's start back to cover cur */
-			ext->oe_start = cur->oe_start;
-			ext->oe_grants += chunksize;
-			LASSERT(*grants >= chunksize);
+		/* On success, osc_extent_merge() will put cur,
+		 * so we take an extra reference
+		 */
+		osc_extent_get(cur);
+		if (osc_extent_merge(env, ext, cur) == 0) {
 			*grants -= chunksize;
-
 			found = osc_extent_hold(ext);
-		} else if (chunk == ext_chk_end + 1) {
-			/* rear merge */
-			ext->oe_end = cur->oe_end;
-			ext->oe_grants += chunksize;
-			LASSERT(*grants >= chunksize);
-			*grants -= chunksize;
 
-			/* try to merge with the next one because we just fill
-			 * in a gap
+			/*
+			 * Try to merge with the next one too because we
+			 * might have just filled in a gap.
 			 */
 			if (osc_extent_merge(env, ext, next_extent(ext)) == 0)
 				/* we can save extent tax from next extent */
 				*grants += cli->cl_grant_extent_tax;
 
-			found = osc_extent_hold(ext);
-		}
-		if (found)
 			break;
+		}
+		osc_extent_put(env, cur);
 	}
 
 	osc_extent_tree_dump(D_CACHE, obj);

[lustre-devel] [PATCH 03/29] lustre: osc: simplify osc_extent_wait()

[NeilBrown]

Taking a spinlock to check the current value of the state is
unnecessary.
The wake_up() and wait_event() calls have sufficient barriers
to ensure that the value will be seen and the wait will abort
properly.

In most cases, osc_extent_wait() is followed by osc_object_lock()
before any shared data is touched - in those cases there is no need
for osc_extent_wait() to wait for the spinlock to be released.

The one case where osc_object_lock() does not immediately follow is
in osc_cache_truncate_start().  The extra locking was introduced in a
patch which fixed a problem with truncation, so it is likely that this
is the call that was thought to be relevant.
In that case, following osc_extent_wait(), an extent that had been
detached from the per-object list (oe_link linkage) and proceeds to
work on it without any locking.
In this case the code is waiting for OES_TRUNC, so any changes that
happen after the osc_extent_state_set(ext, OES_TRUNC) and when the
lock is dropped, might not be seen by the woken code.
The only thing changed is ->oe_trunc_pending, and the woken code
doesn't look at that.

The only remaining possible need for extra synchronization is if some
other value was changed before the wakeup and is needed after the
wait.  According to memory-barriers.txt, a barrier might be needed
to ensure that is visible.  Such a barrier is most clearly presented
by used smp_store_release() to set the state before wakeup, and
smp_load_acquire() to view it after waiting.

Also use a simple wake_up() instead of wake_up_all() - the latter is
only needed when exclusive waiting is being used.

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

diff --git a/drivers/staging/lustre/lustre/osc/osc_cache.c b/drivers/staging/lustre/lustre/osc/osc_cache.c
index 1ce9f673f1bf..00056dffceb9 100644
--- a/drivers/staging/lustre/lustre/osc/osc_cache.c
+++ b/drivers/staging/lustre/lustre/osc/osc_cache.c
@@ -345,8 +345,8 @@ static void osc_extent_state_set(struct osc_extent *ext, int state)
 	/* LASSERT(sanity_check_nolock(ext) == 0); */
 
 	/* TODO: validate the state machine */
-	ext->oe_state = state;
-	wake_up_all(&ext->oe_waitq);
+	smp_store_release(&ext->oe_state, state);
+	wake_up(&ext->oe_waitq);
 }
 
 static struct osc_extent *osc_extent_alloc(struct osc_object *obj)
@@ -948,17 +948,6 @@ int osc_extent_finish(const struct lu_env *env, struct osc_extent *ext,
 	return 0;
 }
 
-static int extent_wait_cb(struct osc_extent *ext, enum osc_extent_state state)
-{
-	int ret;
-
-	osc_object_lock(ext->oe_obj);
-	ret = ext->oe_state == state;
-	osc_object_unlock(ext->oe_obj);
-
-	return ret;
-}
-
 /**
  * Wait for the extent's state to become @state.
  */
@@ -989,13 +978,16 @@ static int osc_extent_wait(const struct lu_env *env, struct osc_extent *ext,
 
 	/* wait for the extent until its state becomes @state */
 	rc = wait_event_idle_timeout(ext->oe_waitq,
-				     extent_wait_cb(ext, state), 600 * HZ);
+				     smp_load_acquire(&ext->oe_state) == state,
+				     600 * HZ);
 	if (rc == 0) {
 		OSC_EXTENT_DUMP(D_ERROR, ext,
 				"%s: wait ext to %u timedout, recovery in progress?\n",
 				cli_name(osc_cli(obj)), state);
 
-		wait_event_idle(ext->oe_waitq, extent_wait_cb(ext, state));
+		wait_event_idle(ext->oe_waitq,
+				smp_load_acquire(&ext->oe_state) == state);
+
 	}
 	if (ext->oe_rc < 0)
 		rc = ext->oe_rc;

[lustre-devel] [PATCH 13/29] lustre: osc_cache: change osc_enter_cache_try to return bool.

[NeilBrown]

'bool' is the natural type for this, so make that obvious.

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

diff --git a/drivers/staging/lustre/lustre/osc/osc_cache.c b/drivers/staging/lustre/lustre/osc/osc_cache.c
index 5a295799f177..e9987c187ecd 100644
--- a/drivers/staging/lustre/lustre/osc/osc_cache.c
+++ b/drivers/staging/lustre/lustre/osc/osc_cache.c
@@ -1533,17 +1533,14 @@ static void osc_exit_cache(struct client_obd *cli, struct osc_async_page *oap)
  * Non-blocking version of osc_enter_cache() that consumes grant only when it
  * is available.
  */
-static int osc_enter_cache_try(struct client_obd *cli,
-			       struct osc_async_page *oap,
-			       int bytes, int transient)
+static bool osc_enter_cache_try(struct client_obd *cli,
+				struct osc_async_page *oap,
+				int bytes, int transient)
 {
-	int rc;
-
 	OSC_DUMP_GRANT(D_CACHE, cli, "need:%d\n", bytes);
 
-	rc = osc_reserve_grant(cli, bytes);
-	if (rc < 0)
-		return 0;
+	if (osc_reserve_grant(cli, bytes) < 0)
+		return false;
 
 	if (cli->cl_dirty_pages < cli->cl_dirty_max_pages &&
 	    atomic_long_read(&obd_dirty_pages) + 1 <= obd_max_dirty_pages) {
@@ -1553,12 +1550,11 @@ static int osc_enter_cache_try(struct client_obd *cli,
 			atomic_long_inc(&obd_dirty_transit_pages);
 			oap->oap_brw_flags |= OBD_BRW_NOCACHE;
 		}
-		rc = 1;
+		return true;
 	} else {
 		__osc_unreserve_grant(cli, bytes, bytes);
-		rc = 0;
+		return false;
 	}
-	return rc;
 }
 
 static int ocw_granted(struct client_obd *cli, struct osc_cache_waiter *ocw)
@@ -2457,12 +2453,12 @@ 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);
-		rc = osc_enter_cache_try(cli, oap, grants, 0);
-		spin_unlock(&cli->cl_loi_list_lock);
-		if (rc == 0) { /* try failed */
+		if (!osc_enter_cache_try(cli, oap, grants, 0)) {
 			grants = 0;
 			need_release = 1;
-		} else if (ext->oe_end < index) {
+		}
+		spin_unlock(&cli->cl_loi_list_lock);
+		if (!need_release && ext->oe_end < index) {
 			tmp = grants;
 			/* try to expand this extent */
 			rc = osc_extent_expand(ext, index, &tmp);

[lustre-devel] [PATCH 18/29] lustre: osc_cache: avoid unnecessary tests.

[NeilBrown]

These tests (have we reached page limit) are not necessary, as the
next call to try_to_add_extent_for_io() will perform the
same test, and fail if necessary.

Having the tests don't add any clarity to the code, and are unlikely
to help performance as they are likely to fail more often than they
succeed.

So discard them.

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

diff --git a/drivers/staging/lustre/lustre/osc/osc_cache.c b/drivers/staging/lustre/lustre/osc/osc_cache.c
index 14e6efb4b80b..5666f384ac6c 100644
--- a/drivers/staging/lustre/lustre/osc/osc_cache.c
+++ b/drivers/staging/lustre/lustre/osc/osc_cache.c
@@ -1990,8 +1990,6 @@ static unsigned int get_write_extents(struct osc_object *obj,
 			return data.erd_page_count;
 		EASSERT(ext->oe_nr_pages <= data.erd_max_pages, ext);
 	}
-	if (data.erd_page_count == data.erd_max_pages)
-		return data.erd_page_count;
 
 	while (!list_empty(&obj->oo_urgent_exts)) {
 		ext = list_entry(obj->oo_urgent_exts.next,
@@ -1999,8 +1997,6 @@ static unsigned int get_write_extents(struct osc_object *obj,
 		if (!try_to_add_extent_for_io(cli, ext, &data))
 			return data.erd_page_count;
 	}
-	if (data.erd_page_count == data.erd_max_pages)
-		return data.erd_page_count;
 
 	/*
 	 * One key difference between full extents and other extents: full
@@ -2015,8 +2011,6 @@ static unsigned int get_write_extents(struct osc_object *obj,
 		if (!try_to_add_extent_for_io(cli, ext, &data))
 			break;
 	}
-	if (data.erd_page_count == data.erd_max_pages)
-		return data.erd_page_count;
 
 	ext = first_extent(obj);
 	while (ext) {

[lustre-devel] [PATCH 21/29] lustre: osc_cache: don't drop a lock we didn't take - two

[NeilBrown]

osc_check_rpcs() drops a lock that it was called with, which can make
it harder to understand the code.

It is only called from one place, and that places takes the lock just
to all this function.
So instead, take the lock at the start of the function,
and drop it at the end.
This makes the code easier to follow.

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

diff --git a/drivers/staging/lustre/lustre/osc/osc_cache.c b/drivers/staging/lustre/lustre/osc/osc_cache.c
index 863884cac028..b2ad6a15014e 100644
--- a/drivers/staging/lustre/lustre/osc/osc_cache.c
+++ b/drivers/staging/lustre/lustre/osc/osc_cache.c
@@ -2187,11 +2187,11 @@ static struct osc_object *osc_next_obj(struct client_obd *cli)
 
 /* called with the loi list lock held */
 static void osc_check_rpcs(const struct lu_env *env, struct client_obd *cli)
-	__must_hold(&cli->cl_loi_list_lock)
 {
 	struct osc_object *osc;
 	int rc = 0;
 
+	spin_lock(&cli->cl_loi_list_lock);
 	while ((osc = osc_next_obj(cli)) != NULL) {
 		struct cl_object *obj = osc2cl(osc);
 		struct lu_ref_link link;
@@ -2247,6 +2247,7 @@ static void osc_check_rpcs(const struct lu_env *env, struct client_obd *cli)
 
 		spin_lock(&cli->cl_loi_list_lock);
 	}
+	spin_unlock(&cli->cl_loi_list_lock);
 }
 
 static int osc_io_unplug0(const struct lu_env *env, struct client_obd *cli,
@@ -2258,9 +2259,7 @@ static int osc_io_unplug0(const struct lu_env *env, struct client_obd *cli,
 		return 0;
 
 	if (!async) {
-		spin_lock(&cli->cl_loi_list_lock);
 		osc_check_rpcs(env, cli);
-		spin_unlock(&cli->cl_loi_list_lock);
 	} else {
 		CDEBUG(D_CACHE, "Queue writeback work for client %p.\n", cli);
 		LASSERT(cli->cl_writeback_work);

[lustre-devel] [PATCH 21/29] lustre: osc_cache: don't drop a lock we didn't take - two

[Andreas Dilger]

> On Jan 8, 2019, at 23:24, NeilBrown <neilb@suse.com> wrote:
> 
> osc_check_rpcs() drops a lock that it was called with, which can make
> it harder to understand the code.
> 
> It is only called from one place, and that places takes the lock just
> to all this function.

(typo) "call"

> So instead, take the lock at the start of the function,
> and drop it at the end.
> This makes the code easier to follow.
> 
> Signed-off-by: NeilBrown <neilb@suse.com>

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

> ---
> drivers/staging/lustre/lustre/osc/osc_cache.c |    5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/staging/lustre/lustre/osc/osc_cache.c b/drivers/staging/lustre/lustre/osc/osc_cache.c
> index 863884cac028..b2ad6a15014e 100644
> --- a/drivers/staging/lustre/lustre/osc/osc_cache.c
> +++ b/drivers/staging/lustre/lustre/osc/osc_cache.c
> @@ -2187,11 +2187,11 @@ static struct osc_object *osc_next_obj(struct client_obd *cli)
> 
> /* called with the loi list lock held */
> static void osc_check_rpcs(const struct lu_env *env, struct client_obd *cli)
> -	__must_hold(&cli->cl_loi_list_lock)
> {
> 	struct osc_object *osc;
> 	int rc = 0;
> 
> +	spin_lock(&cli->cl_loi_list_lock);
> 	while ((osc = osc_next_obj(cli)) != NULL) {
> 		struct cl_object *obj = osc2cl(osc);
> 		struct lu_ref_link link;
> @@ -2247,6 +2247,7 @@ static void osc_check_rpcs(const struct lu_env *env, struct client_obd *cli)
> 
> 		spin_lock(&cli->cl_loi_list_lock);
> 	}
> +	spin_unlock(&cli->cl_loi_list_lock);
> }
> 
> static int osc_io_unplug0(const struct lu_env *env, struct client_obd *cli,
> @@ -2258,9 +2259,7 @@ static int osc_io_unplug0(const struct lu_env *env, struct client_obd *cli,
> 		return 0;
> 
> 	if (!async) {
> -		spin_lock(&cli->cl_loi_list_lock);
> 		osc_check_rpcs(env, cli);
> -		spin_unlock(&cli->cl_loi_list_lock);
> 	} else {
> 		CDEBUG(D_CACHE, "Queue writeback work for client %p.\n", cli);
> 		LASSERT(cli->cl_writeback_work);
> 
> 

Cheers, Andreas
---
Andreas Dilger
CTO Whamcloud

[lustre-devel] [PATCH 20/29] lustre: osc_cache: don't drop a lock we didn't take.

[NeilBrown]

Dropping a lock in a function which didn't take the lock
is best avoided as it makes it difficult to understand the code.
Sometimes it is unavoidable, but not in this case.

There is very little code in the (only) calling function which is also
locked, so we can move that code into the called function, and
then just take the lock inside the called function - the same function
which drops it.

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

diff --git a/drivers/staging/lustre/lustre/osc/osc_cache.c b/drivers/staging/lustre/lustre/osc/osc_cache.c
index db28cc4d5ae8..863884cac028 100644
--- a/drivers/staging/lustre/lustre/osc/osc_cache.c
+++ b/drivers/staging/lustre/lustre/osc/osc_cache.c
@@ -2029,7 +2029,6 @@ static unsigned int get_write_extents(struct osc_object *obj,
 static int
 osc_send_write_rpc(const struct lu_env *env, struct client_obd *cli,
 		   struct osc_object *osc)
-	__must_hold(osc)
 {
 	LIST_HEAD(rpclist);
 	struct osc_extent *ext;
@@ -2039,13 +2038,16 @@ osc_send_write_rpc(const struct lu_env *env, struct client_obd *cli,
 	int srvlock = 0;
 	int rc = 0;
 
-	assert_osc_object_is_locked(osc);
+	osc_object_lock(osc);
+	if (osc_makes_rpc(cli, osc, OBD_BRW_WRITE))
+		page_count = get_write_extents(osc, &rpclist);
 
-	page_count = get_write_extents(osc, &rpclist);
 	LASSERT(equi(page_count == 0, list_empty(&rpclist)));
 
-	if (list_empty(&rpclist))
+	if (list_empty(&rpclist)) {
+		osc_object_unlock(osc);
 		return 0;
+	}
 
 	osc_update_pending(osc, OBD_BRW_WRITE, -page_count);
 
@@ -2086,7 +2088,6 @@ osc_send_write_rpc(const struct lu_env *env, struct client_obd *cli,
 		LASSERT(list_empty(&rpclist));
 	}
 
-	osc_object_lock(osc);
 	return rc;
 }
 
@@ -2103,7 +2104,6 @@ osc_send_write_rpc(const struct lu_env *env, struct client_obd *cli,
 static int
 osc_send_read_rpc(const struct lu_env *env, struct client_obd *cli,
 		  struct osc_object *osc)
-	__must_hold(osc)
 {
 	struct osc_extent *ext;
 	struct osc_extent *next;
@@ -2117,7 +2117,12 @@ osc_send_read_rpc(const struct lu_env *env, struct client_obd *cli,
 	};
 	int rc = 0;
 
-	assert_osc_object_is_locked(osc);
+	osc_object_lock(osc);
+	if (!osc_makes_rpc(cli, osc, OBD_BRW_READ)) {
+		osc_object_unlock(osc);
+		return rc;
+	}
+
 	list_for_each_entry_safe(ext, next, &osc->oo_reading_exts, oe_link) {
 		EASSERT(ext->oe_state == OES_LOCK_DONE, ext);
 		if (!try_to_add_extent_for_io(cli, ext, &data))
@@ -2129,13 +2134,12 @@ osc_send_read_rpc(const struct lu_env *env, struct client_obd *cli,
 
 	osc_update_pending(osc, OBD_BRW_READ, -data.erd_page_count);
 
+	osc_object_unlock(osc);
 	if (!list_empty(&rpclist)) {
-		osc_object_unlock(osc);
 
 		rc = osc_build_rpc(env, cli, &rpclist, OBD_BRW_READ);
 		LASSERT(list_empty(&rpclist));
 
-		osc_object_lock(osc);
 	}
 	return rc;
 }
@@ -2210,38 +2214,32 @@ static void osc_check_rpcs(const struct lu_env *env, struct client_obd *cli)
 		 * partial read pending queue when we're given this object to
 		 * do io on writes while there are cache waiters
 		 */
-		osc_object_lock(osc);
-		if (osc_makes_rpc(cli, osc, OBD_BRW_WRITE)) {
-			rc = osc_send_write_rpc(env, cli, osc);
-			if (rc < 0) {
-				CERROR("Write request failed with %d\n", rc);
-
-				/* osc_send_write_rpc failed, mostly because of
-				 * memory pressure.
-				 *
-				 * It can't break here, because if:
-				 *  - a page was submitted by osc_io_submit, so
-				 *    page locked;
-				 *  - no request in flight
-				 *  - no subsequent request
-				 * The system will be in live-lock state,
-				 * because there is no chance to call
-				 * osc_io_unplug() and osc_check_rpcs() any
-				 * more. pdflush can't help in this case,
-				 * because it might be blocked at grabbing
-				 * the page lock as we mentioned.
-				 *
-				 * Anyway, continue to drain pages.
-				 */
-				/* break; */
-			}
-		}
-		if (osc_makes_rpc(cli, osc, OBD_BRW_READ)) {
-			rc = osc_send_read_rpc(env, cli, osc);
-			if (rc < 0)
-				CERROR("Read request failed with %d\n", rc);
+		rc = osc_send_write_rpc(env, cli, osc);
+		if (rc < 0) {
+			CERROR("Write request failed with %d\n", rc);
+
+			/* osc_send_write_rpc failed, mostly because of
+			 * memory pressure.
+			 *
+			 * It can't break here, because if:
+			 *  - a page was submitted by osc_io_submit, so
+			 *    page locked;
+			 *  - no request in flight
+			 *  - no subsequent request
+			 * The system will be in live-lock state,
+			 * because there is no chance to call
+			 * osc_io_unplug() and osc_check_rpcs() any
+			 * more. pdflush can't help in this case,
+			 * because it might be blocked@grabbing
+			 * the page lock as we mentioned.
+			 *
+			 * Anyway, continue to drain pages.
+			 */
+			/* break; */
 		}
-		osc_object_unlock(osc);
+		rc = osc_send_read_rpc(env, cli, osc);
+		if (rc < 0)
+			CERROR("Read request failed with %d\n", rc);
 
 		osc_list_maint(cli, osc);
 		lu_object_ref_del_at(&obj->co_lu, &link, "check", current);

[lustre-devel] [PATCH 17/29] lustre: osc_cache: simplify list walk in get_write_extents().

[NeilBrown]

This iteration is exactly what list_first_entry_or_null() is for.

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

diff --git a/drivers/staging/lustre/lustre/osc/osc_cache.c b/drivers/staging/lustre/lustre/osc/osc_cache.c
index d2d5867156ad..14e6efb4b80b 100644
--- a/drivers/staging/lustre/lustre/osc/osc_cache.c
+++ b/drivers/staging/lustre/lustre/osc/osc_cache.c
@@ -1982,9 +1982,9 @@ static unsigned int get_write_extents(struct osc_object *obj,
 	};
 
 	assert_osc_object_is_locked(obj);
-	while (!list_empty(&obj->oo_hp_exts)) {
-		ext = list_entry(obj->oo_hp_exts.next, struct osc_extent,
-				 oe_link);
+	while ((ext = list_first_entry_or_null(&obj->oo_hp_exts,
+					       struct osc_extent,
+					       oe_link))) {
 		LASSERT(ext->oe_state == OES_CACHE);
 		if (!try_to_add_extent_for_io(cli, ext, &data))
 			return data.erd_page_count;

[lustre-devel] [PATCH 24/29] lustre: osc_cache: change need_release to bool

[NeilBrown]

This variable is used like a bool, so declare it as one.

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

diff --git a/drivers/staging/lustre/lustre/osc/osc_cache.c b/drivers/staging/lustre/lustre/osc/osc_cache.c
index b4bb36926046..1476f84e6156 100644
--- a/drivers/staging/lustre/lustre/osc/osc_cache.c
+++ b/drivers/staging/lustre/lustre/osc/osc_cache.c
@@ -2316,7 +2316,7 @@ int osc_queue_async_io(const struct lu_env *env, struct cl_io *io,
 	unsigned int grants = 0, tmp;
 	int brw_flags = OBD_BRW_ASYNC;
 	int cmd = OBD_BRW_WRITE;
-	int need_release = 0;
+	bool need_release = false;
 	int rc = 0;
 
 	if (oap->oap_magic != OAP_MAGIC)
@@ -2393,7 +2393,7 @@ int osc_queue_async_io(const struct lu_env *env, struct cl_io *io,
 		spin_lock(&cli->cl_loi_list_lock);
 		if (!osc_enter_cache_try(cli, oap, grants)) {
 			grants = 0;
-			need_release = 1;
+			need_release = true;
 		}
 		spin_unlock(&cli->cl_loi_list_lock);
 		if (!need_release && ext->oe_end < index) {
@@ -2401,7 +2401,7 @@ int osc_queue_async_io(const struct lu_env *env, struct cl_io *io,
 			/* try to expand this extent */
 			rc = osc_extent_expand(ext, index, &tmp);
 			if (rc < 0) {
-				need_release = 1;
+				need_release = true;
 				/* don't free reserved grant */
 			} else {
 				OSC_EXTENT_DUMP(D_CACHE, ext,
@@ -2413,7 +2413,7 @@ int osc_queue_async_io(const struct lu_env *env, struct cl_io *io,
 		rc = 0;
 	} else if (ext) {
 		/* index is located outside of active extent */
-		need_release = 1;
+		need_release = true;
 	}
 	if (need_release) {
 		osc_extent_release(env, ext);

[lustre-devel] [PATCH 24/29] lustre: osc_cache: change need_release to bool

[Andreas Dilger]

On Jan 8, 2019, at 23:24, NeilBrown <neilb@suse.com> wrote:
> 
> This variable is used like a bool, so declare it as one.
> 
> Signed-off-by: NeilBrown <neilb@suse.com>

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

> ---
> drivers/staging/lustre/lustre/osc/osc_cache.c |    8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/lustre/lustre/osc/osc_cache.c b/drivers/staging/lustre/lustre/osc/osc_cache.c
> index b4bb36926046..1476f84e6156 100644
> --- a/drivers/staging/lustre/lustre/osc/osc_cache.c
> +++ b/drivers/staging/lustre/lustre/osc/osc_cache.c
> @@ -2316,7 +2316,7 @@ int osc_queue_async_io(const struct lu_env *env, struct cl_io *io,
> 	unsigned int grants = 0, tmp;
> 	int brw_flags = OBD_BRW_ASYNC;
> 	int cmd = OBD_BRW_WRITE;
> -	int need_release = 0;
> +	bool need_release = false;
> 	int rc = 0;
> 
> 	if (oap->oap_magic != OAP_MAGIC)
> @@ -2393,7 +2393,7 @@ int osc_queue_async_io(const struct lu_env *env, struct cl_io *io,
> 		spin_lock(&cli->cl_loi_list_lock);
> 		if (!osc_enter_cache_try(cli, oap, grants)) {
> 			grants = 0;
> -			need_release = 1;
> +			need_release = true;
> 		}
> 		spin_unlock(&cli->cl_loi_list_lock);
> 		if (!need_release && ext->oe_end < index) {
> @@ -2401,7 +2401,7 @@ int osc_queue_async_io(const struct lu_env *env, struct cl_io *io,
> 			/* try to expand this extent */
> 			rc = osc_extent_expand(ext, index, &tmp);
> 			if (rc < 0) {
> -				need_release = 1;
> +				need_release = true;
> 				/* don't free reserved grant */
> 			} else {
> 				OSC_EXTENT_DUMP(D_CACHE, ext,
> @@ -2413,7 +2413,7 @@ int osc_queue_async_io(const struct lu_env *env, struct cl_io *io,
> 		rc = 0;
> 	} else if (ext) {
> 		/* index is located outside of active extent */
> -		need_release = 1;
> +		need_release = true;
> 	}
> 	if (need_release) {
> 		osc_extent_release(env, ext);
> 
> 

Cheers, Andreas
---
Andreas Dilger
Principal Lustre Architect
Whamcloud

[lustre-devel] [PATCH 19/29] lustre: osc_cache: convert while to for in get_write_extents()

[NeilBrown]

This 'while' loop is really a 'for' loop in disguise.
So change it to 'for'.

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

diff --git a/drivers/staging/lustre/lustre/osc/osc_cache.c b/drivers/staging/lustre/lustre/osc/osc_cache.c
index 5666f384ac6c..db28cc4d5ae8 100644
--- a/drivers/staging/lustre/lustre/osc/osc_cache.c
+++ b/drivers/staging/lustre/lustre/osc/osc_cache.c
@@ -2012,19 +2012,16 @@ static unsigned int get_write_extents(struct osc_object *obj,
 			break;
 	}
 
-	ext = first_extent(obj);
-	while (ext) {
+	for (ext = first_extent(obj);
+	     ext;
+	     ext = next_extent(ext)) {
 		if ((ext->oe_state != OES_CACHE) ||
 		    /* this extent may be already in current rpclist */
-		    (!list_empty(&ext->oe_link) && ext->oe_owner)) {
-			ext = next_extent(ext);
+		    (!list_empty(&ext->oe_link) && ext->oe_owner))
 			continue;
-		}
 
 		if (!try_to_add_extent_for_io(cli, ext, &data))
 			return data.erd_page_count;
-
-		ext = next_extent(ext);
 	}
 	return data.erd_page_count;
 }

[lustre-devel] [PATCH 27/29] lustre: osc_cache: white-space and other checkpatch fixes.

[NeilBrown]

Assorted minor checkpatch issues fixed.

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

diff --git a/drivers/staging/lustre/lustre/osc/osc_cache.c b/drivers/staging/lustre/lustre/osc/osc_cache.c
index e01f3815978c..019854b78277 100644
--- a/drivers/staging/lustre/lustre/osc/osc_cache.c
+++ b/drivers/staging/lustre/lustre/osc/osc_cache.c
@@ -101,7 +101,7 @@ static inline char list_empty_marker(struct list_head *list)
 
 #define EXTSTR       "[%lu -> %lu/%lu]"
 #define EXTPARA(ext) (ext)->oe_start, (ext)->oe_end, (ext)->oe_max_end
-static const char *oes_strings[] = {
+static const char * const oes_strings[] = {
 	"inv", "active", "cache", "locking", "lockdone", "rpc", "trunc", NULL };
 
 #define OSC_EXTENT_DUMP(lvl, extent, fmt, ...) do {			      \
@@ -668,7 +668,7 @@ static struct osc_extent *osc_extent_find(const struct lu_env *env,
 					  unsigned int *grants)
 {
 	struct client_obd *cli = osc_cli(obj);
-	struct osc_lock   *olck;
+	struct osc_lock *olck;
 	struct cl_lock_descr *descr;
 	struct osc_extent *cur;
 	struct osc_extent *ext;
@@ -962,7 +962,6 @@ static int osc_extent_wait(const struct lu_env *env, struct osc_extent *ext,
 
 		wait_event_idle(ext->oe_waitq,
 				smp_load_acquire(&ext->oe_state) == state);
-
 	}
 	return ext->oe_rc < 0 ? ext->oe_rc : 0;
 }
@@ -1020,7 +1019,8 @@ static int osc_extent_truncate(struct osc_extent *ext, pgoff_t trunc_index,
 		if (index < trunc_index ||
 		    (index == trunc_index && partial)) {
 			/* accounting how many pages remaining in the chunk
-			 * so that we can calculate grants correctly. */
+			 * so that we can calculate grants correctly.
+			 */
 			if (index >> ppc_bits == trunc_chunk)
 				++pages_in_chunk;
 			continue;
@@ -1141,7 +1141,8 @@ static int osc_extent_make_ready(const struct lu_env *env,
 	 * the size of file.
 	 */
 	if (!(last->oap_async_flags & ASYNC_COUNT_STABLE)) {
-		int last_oap_count = osc_refresh_count(env, last, OBD_BRW_WRITE);
+		int last_oap_count = osc_refresh_count(env, last,
+						       OBD_BRW_WRITE);
 
 		LASSERT(last_oap_count > 0);
 		LASSERT(last->oap_page_off + last_oap_count <= PAGE_SIZE);
@@ -1337,7 +1338,7 @@ static int osc_completion(const struct lu_env *env, struct osc_async_page *oap,
 			  int cmd, int rc)
 {
 	struct osc_page *opg = oap2osc_page(oap);
-	struct cl_page    *page = oap2cl_page(oap);
+	struct cl_page *page = oap2cl_page(oap);
 	enum cl_req_type crt;
 	int srvlock;
 
@@ -1736,7 +1737,8 @@ static void osc_update_pending(struct osc_object *obj, int cmd, int delta)
 	OSC_IO_DEBUG(obj, "update pending cmd %d delta %d.\n", cmd, delta);
 }
 
-static void on_list(struct list_head *item, struct list_head *list, int should_be_on)
+static void on_list(struct list_head *item, struct list_head *list,
+		    int should_be_on)
 {
 	if (list_empty(item) && should_be_on)
 		list_add_tail(item, list);
@@ -1898,7 +1900,8 @@ static int try_to_add_extent_for_io(struct client_obd *cli,
 					oap_pending_item);
 		EASSERT(tmp->oe_owner == current, tmp);
 		if (oap2cl_page(oap)->cp_type != oap2cl_page(oap2)->cp_type) {
-			CDEBUG(D_CACHE, "Do not permit different type of IO in one RPC\n");
+			CDEBUG(D_CACHE,
+			       "Do not permit different type of IO in one RPC\n");
 			return 0;
 		}
 
@@ -2130,10 +2133,8 @@ osc_send_read_rpc(const struct lu_env *env, struct client_obd *cli,
 
 	osc_object_unlock(osc);
 	if (!list_empty(&rpclist)) {
-
 		rc = osc_build_rpc(env, cli, &rpclist, OBD_BRW_READ);
 		LASSERT(list_empty(&rpclist));
-
 	}
 	return rc;
 }
@@ -3082,7 +3083,7 @@ bool osc_page_gang_lookup(const struct lu_env *env, struct cl_io *io,
  * Check if page @page is covered by an extra lock or discard it.
  */
 static bool check_and_discard_cb(const struct lu_env *env, struct cl_io *io,
-				struct osc_page *ops, void *cbdata)
+				 struct osc_page *ops, void *cbdata)
 {
 	struct osc_thread_info *info = osc_env_info(env);
 	struct osc_object *osc = cbdata;
@@ -3121,7 +3122,7 @@ static bool check_and_discard_cb(const struct lu_env *env, struct cl_io *io,
 }
 
 static bool discard_cb(const struct lu_env *env, struct cl_io *io,
-		      struct osc_page *ops, void *cbdata)
+		       struct osc_page *ops, void *cbdata)
 {
 	struct osc_thread_info *info = osc_env_info(env);
 	struct cl_page *page = ops->ops_cl.cpl_page;

[lustre-devel] [PATCH 27/29] lustre: osc_cache: white-space and other checkpatch fixes.

[Andreas Dilger]

On Jan 8, 2019, at 23:24, NeilBrown <neilb@suse.com> wrote:
> 
> Assorted minor checkpatch issues fixed.
> 
> Signed-off-by: NeilBrown <neilb@suse.com>
> ---
> drivers/staging/lustre/lustre/osc/osc_cache.c |   25 +++++++++++++------------
> 1 file changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/staging/lustre/lustre/osc/osc_cache.c b/drivers/staging/lustre/lustre/osc/osc_cache.c
> index e01f3815978c..019854b78277 100644
> --- a/drivers/staging/lustre/lustre/osc/osc_cache.c
> +++ b/drivers/staging/lustre/lustre/osc/osc_cache.c
> @@ -962,7 +962,6 @@ static int osc_extent_wait(const struct lu_env *env, struct osc_extent *ext,
> 
> 		wait_event_idle(ext->oe_waitq,
> 				smp_load_acquire(&ext->oe_state) == state);
> -
> 	}
> 	return ext->oe_rc < 0 ? ext->oe_rc : 0;
> }

Typically we prefer a blank line before return.  Otherwise fine.

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

> @@ -1020,7 +1019,8 @@ static int osc_extent_truncate(struct osc_extent *ext, pgoff_t trunc_index,
> 		if (index < trunc_index ||
> 		    (index == trunc_index && partial)) {
> 			/* accounting how many pages remaining in the chunk
> -			 * so that we can calculate grants correctly. */
> +			 * so that we can calculate grants correctly.
> +			 */
> 			if (index >> ppc_bits == trunc_chunk)
> 				++pages_in_chunk;
> 			continue;
> @@ -1141,7 +1141,8 @@ static int osc_extent_make_ready(const struct lu_env *env,
> 	 * the size of file.
> 	 */
> 	if (!(last->oap_async_flags & ASYNC_COUNT_STABLE)) {
> -		int last_oap_count = osc_refresh_count(env, last, OBD_BRW_WRITE);
> +		int last_oap_count = osc_refresh_count(env, last,
> +						       OBD_BRW_WRITE);
> 
> 		LASSERT(last_oap_count > 0);
> 		LASSERT(last->oap_page_off + last_oap_count <= PAGE_SIZE);
> @@ -1337,7 +1338,7 @@ static int osc_completion(const struct lu_env *env, struct osc_async_page *oap,
> 			  int cmd, int rc)
> {
> 	struct osc_page *opg = oap2osc_page(oap);
> -	struct cl_page    *page = oap2cl_page(oap);
> +	struct cl_page *page = oap2cl_page(oap);
> 	enum cl_req_type crt;
> 	int srvlock;
> 
> @@ -1736,7 +1737,8 @@ static void osc_update_pending(struct osc_object *obj, int cmd, int delta)
> 	OSC_IO_DEBUG(obj, "update pending cmd %d delta %d.\n", cmd, delta);
> }
> 
> -static void on_list(struct list_head *item, struct list_head *list, int should_be_on)
> +static void on_list(struct list_head *item, struct list_head *list,
> +		    int should_be_on)
> {
> 	if (list_empty(item) && should_be_on)
> 		list_add_tail(item, list);
> @@ -1898,7 +1900,8 @@ static int try_to_add_extent_for_io(struct client_obd *cli,
> 					oap_pending_item);
> 		EASSERT(tmp->oe_owner == current, tmp);
> 		if (oap2cl_page(oap)->cp_type != oap2cl_page(oap2)->cp_type) {
> -			CDEBUG(D_CACHE, "Do not permit different type of IO in one RPC\n");
> +			CDEBUG(D_CACHE,
> +			       "Do not permit different type of IO in one RPC\n");
> 			return 0;
> 		}
> 
> @@ -2130,10 +2133,8 @@ osc_send_read_rpc(const struct lu_env *env, struct client_obd *cli,
> 
> 	osc_object_unlock(osc);
> 	if (!list_empty(&rpclist)) {
> -
> 		rc = osc_build_rpc(env, cli, &rpclist, OBD_BRW_READ);
> 		LASSERT(list_empty(&rpclist));
> -
> 	}
> 	return rc;
> }
> @@ -3082,7 +3083,7 @@ bool osc_page_gang_lookup(const struct lu_env *env, struct cl_io *io,
>  * Check if page @page is covered by an extra lock or discard it.
>  */
> static bool check_and_discard_cb(const struct lu_env *env, struct cl_io *io,
> -				struct osc_page *ops, void *cbdata)
> +				 struct osc_page *ops, void *cbdata)
> {
> 	struct osc_thread_info *info = osc_env_info(env);
> 	struct osc_object *osc = cbdata;
> @@ -3121,7 +3122,7 @@ static bool check_and_discard_cb(const struct lu_env *env, struct cl_io *io,
> }
> 
> static bool discard_cb(const struct lu_env *env, struct cl_io *io,
> -		      struct osc_page *ops, void *cbdata)
> +		       struct osc_page *ops, void *cbdata)
> {
> 	struct osc_thread_info *info = osc_env_info(env);
> 	struct cl_page *page = ops->ops_cl.cpl_page;
> 
> 

Cheers, Andreas
---
Andreas Dilger
Principal Lustre Architect
Whamcloud

[lustre-devel] [PATCH 27/29] lustre: osc_cache: white-space and other checkpatch fixes.

[NeilBrown]

On Thu, Jan 10 2019, Andreas Dilger wrote:

> On Jan 8, 2019, at 23:24, NeilBrown <neilb@suse.com> wrote:
>> 
>> Assorted minor checkpatch issues fixed.
>> 
>> Signed-off-by: NeilBrown <neilb@suse.com>
>> ---
>> drivers/staging/lustre/lustre/osc/osc_cache.c |   25 +++++++++++++------------
>> 1 file changed, 13 insertions(+), 12 deletions(-)
>> 
>> diff --git a/drivers/staging/lustre/lustre/osc/osc_cache.c b/drivers/staging/lustre/lustre/osc/osc_cache.c
>> index e01f3815978c..019854b78277 100644
>> --- a/drivers/staging/lustre/lustre/osc/osc_cache.c
>> +++ b/drivers/staging/lustre/lustre/osc/osc_cache.c
>> @@ -962,7 +962,6 @@ static int osc_extent_wait(const struct lu_env *env, struct osc_extent *ext,
>> 
>> 		wait_event_idle(ext->oe_waitq,
>> 				smp_load_acquire(&ext->oe_state) == state);
>> -
>> 	}
>> 	return ext->oe_rc < 0 ? ext->oe_rc : 0;
>> }
>
> Typically we prefer a blank line before return.  Otherwise fine.

Interesting ... I had a look through osc_cache.c and I can find some
evidence of this preference, but also many cases of the opposite.

While I do see the value in a lot of places, this is one place where I
I'm not so sure.  Specifically, if the line before "return" is a lone "}", it
seems (visually) close enough to a blank line, that adding another line
looks wasteful.
After some debate I have added blank lines there, but not when the
'return' is followed by an 'LASSERT' or other debugging code.

Here is the new version.

Thanks,
NeilBrown

From: NeilBrown <neilb@suse.com>
Subject: [PATCH] lustre: osc_cache: white-space and other checkpatch fixes.

Assorted minor checkpatch issues fixed.

Also add some blank links to help 'return' lines to stand out.

Reviewed-by: Andreas Dilger <adilger@whamcloud.com>
Signed-off-by: NeilBrown <neilb@suse.com>
---
 drivers/staging/lustre/lustre/osc/osc_cache.c | 61 +++++++++++++++++++++------
 1 file changed, 49 insertions(+), 12 deletions(-)

diff --git a/drivers/staging/lustre/lustre/osc/osc_cache.c b/drivers/staging/lustre/lustre/osc/osc_cache.c
index d89edad9fbc5..902e5fd3f501 100644
--- a/drivers/staging/lustre/lustre/osc/osc_cache.c
+++ b/drivers/staging/lustre/lustre/osc/osc_cache.c
@@ -91,6 +91,7 @@ static inline char *ext_flags(struct osc_extent *ext, char *flags)
 	if (ext->oe_fsync_wait)
 		*buf++ = 'Y';
 	*buf = 0;
+
 	return flags;
 }
 
@@ -101,7 +102,7 @@ static inline char list_empty_marker(struct list_head *list)
 
 #define EXTSTR       "[%lu -> %lu/%lu]"
 #define EXTPARA(ext) (ext)->oe_start, (ext)->oe_end, (ext)->oe_max_end
-static const char *oes_strings[] = {
+static const char * const oes_strings[] = {
 	"inv", "active", "cache", "locking", "lockdone", "rpc", "trunc", NULL };
 
 #define OSC_EXTENT_DUMP(lvl, extent, fmt, ...) do {			      \
@@ -337,6 +338,7 @@ static int osc_extent_is_overlapped(struct osc_object *obj,
 		if (overlapped(tmp, ext))
 			return 1;
 	}
+
 	return 0;
 }
 
@@ -404,6 +406,7 @@ static struct osc_extent *osc_extent_get(struct osc_extent *ext)
 {
 	LASSERT(kref_read(&ext->oe_refc) >= 0);
 	kref_get(&ext->oe_refc);
+
 	return ext;
 }
 
@@ -454,6 +457,7 @@ static struct osc_extent *osc_extent_search(struct osc_object *obj,
 			return tmp;
 		}
 	}
+
 	return p;
 }
 
@@ -469,6 +473,7 @@ static struct osc_extent *osc_extent_lookup(struct osc_object *obj,
 	ext = osc_extent_search(obj, index);
 	if (ext && ext->oe_start <= index && index <= ext->oe_end)
 		return osc_extent_get(ext);
+
 	return NULL;
 }
 
@@ -524,6 +529,7 @@ static struct osc_extent *osc_extent_hold(struct osc_extent *ext)
 	}
 	refcount_inc(&ext->oe_users);
 	list_del_init(&ext->oe_link);
+
 	return osc_extent_get(ext);
 }
 
@@ -668,7 +674,7 @@ static struct osc_extent *osc_extent_find(const struct lu_env *env,
 					  unsigned int *grants)
 {
 	struct client_obd *cli = osc_cli(obj);
-	struct osc_lock   *olck;
+	struct osc_lock *olck;
 	struct cl_lock_descr *descr;
 	struct osc_extent *cur;
 	struct osc_extent *ext;
@@ -705,6 +711,7 @@ static struct osc_extent *osc_extent_find(const struct lu_env *env,
 	if ((max_pages & ~chunk_mask) != 0) {
 		CERROR("max_pages: %#x chunkbits: %u chunk_mask: %#lx\n",
 		       max_pages, cli->cl_chunkbits, chunk_mask);
+
 		return ERR_PTR(-EINVAL);
 	}
 	max_end = index - (index % max_pages) + max_pages - 1;
@@ -858,6 +865,7 @@ static struct osc_extent *osc_extent_find(const struct lu_env *env,
 
 out:
 	osc_extent_put(env, cur);
+
 	return found;
 }
 
@@ -919,6 +927,7 @@ int osc_extent_finish(const struct lu_env *env, struct osc_extent *ext,
 	osc_extent_remove(ext);
 	/* put the refcount for RPC */
 	osc_extent_put(env, ext);
+
 	return 0;
 }
 
@@ -962,8 +971,8 @@ static int osc_extent_wait(const struct lu_env *env, struct osc_extent *ext,
 
 		wait_event_idle(ext->oe_waitq,
 				smp_load_acquire(&ext->oe_state) == state);
-
 	}
+
 	return ext->oe_rc < 0 ? ext->oe_rc : 0;
 }
 
@@ -1020,7 +1029,8 @@ static int osc_extent_truncate(struct osc_extent *ext, pgoff_t trunc_index,
 		if (index < trunc_index ||
 		    (index == trunc_index && partial)) {
 			/* accounting how many pages remaining in the chunk
-			 * so that we can calculate grants correctly. */
+			 * so that we can calculate grants correctly.
+			 */
 			if (index >> ppc_bits == trunc_chunk)
 				++pages_in_chunk;
 			continue;
@@ -1087,6 +1097,7 @@ static int osc_extent_truncate(struct osc_extent *ext, pgoff_t trunc_index,
 out:
 	cl_io_fini(env, io);
 	cl_env_put(env, &refcheck);
+
 	return rc;
 }
 
@@ -1141,7 +1152,8 @@ static int osc_extent_make_ready(const struct lu_env *env,
 	 * the size of file.
 	 */
 	if (!(last->oap_async_flags & ASYNC_COUNT_STABLE)) {
-		int last_oap_count = osc_refresh_count(env, last, OBD_BRW_WRITE);
+		int last_oap_count = osc_refresh_count(env, last,
+						       OBD_BRW_WRITE);
 
 		LASSERT(last_oap_count > 0);
 		LASSERT(last->oap_page_off + last_oap_count <= PAGE_SIZE);
@@ -1233,6 +1245,7 @@ static int osc_extent_expand(struct osc_extent *ext, pgoff_t index,
 
 out:
 	osc_object_unlock(obj);
+
 	return rc;
 }
 
@@ -1299,6 +1312,7 @@ static int osc_make_ready(const struct lu_env *env, struct osc_async_page *oap,
 	result = cl_page_make_ready(env, page, CRT_WRITE);
 	if (result == 0)
 		opg->ops_submit_time = jiffies;
+
 	return result;
 }
 
@@ -1322,6 +1336,7 @@ static int osc_refresh_count(const struct lu_env *env,
 	cl_object_attr_unlock(obj);
 	if (result < 0)
 		return result;
+
 	kms = attr->cat_kms;
 	if (cl_offset(obj, index) >= kms)
 		/* catch race with truncate */
@@ -1337,7 +1352,7 @@ static int osc_completion(const struct lu_env *env, struct osc_async_page *oap,
 			  int cmd, int rc)
 {
 	struct osc_page *opg = oap2osc_page(oap);
-	struct cl_page    *page = oap2cl_page(oap);
+	struct cl_page *page = oap2cl_page(oap);
 	enum cl_req_type crt;
 	int srvlock;
 
@@ -1441,6 +1456,7 @@ static int osc_reserve_grant(struct client_obd *cli, unsigned int bytes)
 		cli->cl_reserved_grant += bytes;
 		rc = 0;
 	}
+
 	return rc;
 }
 
@@ -1641,6 +1657,7 @@ static int osc_enter_cache(const struct lu_env *env, struct client_obd *cli,
 	}
 out:
 	spin_unlock(&cli->cl_loi_list_lock);
+
 	return rc;
 }
 
@@ -1731,7 +1748,8 @@ static void osc_update_pending(struct osc_object *obj, int cmd, int delta)
 	OSC_IO_DEBUG(obj, "update pending cmd %d delta %d.\n", cmd, delta);
 }
 
-static void on_list(struct list_head *item, struct list_head *list, int should_be_on)
+static void on_list(struct list_head *item, struct list_head *list,
+		    int should_be_on)
 {
 	if (list_empty(item) && should_be_on)
 		list_add_tail(item, list);
@@ -1790,6 +1808,7 @@ static void osc_process_ar(struct osc_async_rc *ar, __u64 xid,
 
 		ar->ar_force_sync = 1;
 		ar->ar_min_xid = ptlrpc_sample_next_xid();
+
 		return;
 	}
 
@@ -1893,7 +1912,8 @@ static int try_to_add_extent_for_io(struct client_obd *cli,
 					oap_pending_item);
 		EASSERT(tmp->oe_owner == current, tmp);
 		if (oap2cl_page(oap)->cp_type != oap2cl_page(oap2)->cp_type) {
-			CDEBUG(D_CACHE, "Do not permit different type of IO in one RPC\n");
+			CDEBUG(D_CACHE,
+			       "Do not permit different type of IO in one RPC\n");
 			return 0;
 		}
 
@@ -1911,6 +1931,7 @@ static int try_to_add_extent_for_io(struct client_obd *cli,
 	data->erd_page_count += ext->oe_nr_pages;
 	list_move_tail(&ext->oe_link, data->erd_rpc_list);
 	ext->oe_owner = current;
+
 	return 1;
 }
 
@@ -2012,6 +2033,7 @@ static unsigned int get_write_extents(struct osc_object *obj,
 		if (!try_to_add_extent_for_io(cli, ext, &data))
 			return data.erd_page_count;
 	}
+
 	return data.erd_page_count;
 }
 
@@ -2035,6 +2057,7 @@ osc_send_write_rpc(const struct lu_env *env, struct client_obd *cli,
 
 	if (list_empty(&rpclist)) {
 		osc_object_unlock(osc);
+
 		return 0;
 	}
 
@@ -2109,6 +2132,7 @@ osc_send_read_rpc(const struct lu_env *env, struct client_obd *cli,
 	osc_object_lock(osc);
 	if (!osc_makes_rpc(cli, osc, OBD_BRW_READ)) {
 		osc_object_unlock(osc);
+
 		return rc;
 	}
 
@@ -2125,11 +2149,10 @@ osc_send_read_rpc(const struct lu_env *env, struct client_obd *cli,
 
 	osc_object_unlock(osc);
 	if (!list_empty(&rpclist)) {
-
 		rc = osc_build_rpc(env, cli, &rpclist, OBD_BRW_READ);
 		LASSERT(list_empty(&rpclist));
-
 	}
+
 	return rc;
 }
 
@@ -2150,6 +2173,7 @@ static struct osc_object *osc_next_obj(struct client_obd *cli)
 	 */
 	if (!list_empty(&cli->cl_loi_hp_ready_list))
 		return list_to_obj(&cli->cl_loi_hp_ready_list, hp_ready_item);
+
 	if (!list_empty(&cli->cl_loi_ready_list))
 		return list_to_obj(&cli->cl_loi_ready_list, ready_item);
 
@@ -2168,9 +2192,11 @@ static struct osc_object *osc_next_obj(struct client_obd *cli)
 	if (!cli->cl_import || cli->cl_import->imp_invalid) {
 		if (!list_empty(&cli->cl_loi_write_list))
 			return list_to_obj(&cli->cl_loi_write_list, write_item);
+
 		if (!list_empty(&cli->cl_loi_read_list))
 			return list_to_obj(&cli->cl_loi_read_list, read_item);
 	}
+
 	return NULL;
 }
 
@@ -2254,6 +2280,7 @@ static int __osc_io_unplug(const struct lu_env *env, struct client_obd *cli,
 		LASSERT(cli->cl_writeback_work);
 		rc = ptlrpcd_queue_work(cli->cl_writeback_work);
 	}
+
 	return rc;
 }
 
@@ -2295,6 +2322,7 @@ int osc_prep_async_page(struct osc_object *osc, struct osc_page *ops,
 	spin_lock_init(&oap->oap_lock);
 	CDEBUG(D_INFO, "oap %p page %p obj off %llu\n",
 	       oap, page, oap->oap_obj_off);
+
 	return 0;
 }
 
@@ -2464,6 +2492,7 @@ int osc_queue_async_io(const struct lu_env *env, struct cl_io *io,
 		list_add_tail(&oap->oap_pending_item, &ext->oe_pages);
 		osc_object_unlock(osc);
 	}
+
 	return rc;
 }
 
@@ -2499,6 +2528,7 @@ int osc_teardown_async_page(const struct lu_env *env,
 		if (ext)
 			osc_extent_put(env, ext);
 	}
+
 	return rc;
 }
 
@@ -2582,6 +2612,7 @@ int osc_flush_async_page(const struct lu_env *env, struct cl_io *io,
 	osc_extent_put(env, ext);
 	if (unplug)
 		osc_io_unplug_async(env, osc_cli(obj), obj);
+
 	return rc;
 }
 
@@ -2620,6 +2651,7 @@ int osc_queue_sync_pages(const struct lu_env *env, struct osc_object *obj,
 			list_del_init(&oap->oap_pending_item);
 			osc_ap_completion(env, cli, oap, 0, -ENOMEM);
 		}
+
 		return -ENOMEM;
 	}
 
@@ -2649,6 +2681,7 @@ int osc_queue_sync_pages(const struct lu_env *env, struct osc_object *obj,
 	osc_object_unlock(obj);
 
 	osc_io_unplug_async(env, cli, obj);
+
 	return 0;
 }
 
@@ -2777,6 +2810,7 @@ int osc_cache_truncate_start(const struct lu_env *env, struct osc_object *obj,
 		waiting = NULL;
 		goto again;
 	}
+
 	return result;
 }
 
@@ -3070,6 +3104,7 @@ bool osc_page_gang_lookup(const struct lu_env *env, struct cl_io *io,
 	}
 	if (tree_lock)
 		spin_unlock(&osc->oo_tree_lock);
+
 	return res;
 }
 
@@ -3077,7 +3112,7 @@ bool osc_page_gang_lookup(const struct lu_env *env, struct cl_io *io,
  * Check if page @page is covered by an extra lock or discard it.
  */
 static bool check_and_discard_cb(const struct lu_env *env, struct cl_io *io,
-				struct osc_page *ops, void *cbdata)
+				 struct osc_page *ops, void *cbdata)
 {
 	struct osc_thread_info *info = osc_env_info(env);
 	struct osc_object *osc = cbdata;
@@ -3112,11 +3147,12 @@ static bool check_and_discard_cb(const struct lu_env *env, struct cl_io *io,
 	}
 
 	info->oti_next_index = index + 1;
+
 	return true;
 }
 
 static bool discard_cb(const struct lu_env *env, struct cl_io *io,
-		      struct osc_page *ops, void *cbdata)
+		       struct osc_page *ops, void *cbdata)
 {
 	struct osc_thread_info *info = osc_env_info(env);
 	struct cl_page *page = ops->ops_cl.cpl_page;
@@ -3170,6 +3206,7 @@ int osc_lock_discard_pages(const struct lu_env *env, struct osc_object *osc,
 				   info->oti_next_index, end, cb, osc);
 out:
 	cl_io_fini(env, io);
+
 	return result;
 }
 
-- 
2.14.0.rc0.dirty

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <http://lists.lustre.org/pipermail/lustre-devel-lustre.org/attachments/20190111/3edabe33/attachment-0001.sig>

[lustre-devel] [PATCH 15/29] lustre: osc_cache: change osc_make_rpc() to return bool.

[NeilBrown]

'bool' more accurately reflects the nature of this function.

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

diff --git a/drivers/staging/lustre/lustre/osc/osc_cache.c b/drivers/staging/lustre/lustre/osc/osc_cache.c
index ddfb61502f30..9b78562d3366 100644
--- a/drivers/staging/lustre/lustre/osc/osc_cache.c
+++ b/drivers/staging/lustre/lustre/osc/osc_cache.c
@@ -1665,10 +1665,10 @@ static int osc_max_rpc_in_flight(struct client_obd *cli, struct osc_object *osc)
  * (lop).  This is used by osc_check_rpcs->osc_next_obj() and osc_list_maint()
  * to quickly find objects that are ready to send an RPC.
  */
-static int osc_makes_rpc(struct client_obd *cli, struct osc_object *osc,
-			 int cmd)
+static bool osc_makes_rpc(struct client_obd *cli, struct osc_object *osc,
+			  int cmd)
 {
-	int invalid_import = 0;
+	bool invalid_import = false;
 
 	/* if we have an invalid import we want to drain the queued pages
 	 * by forcing them through rpcs that immediately fail and complete
@@ -1676,22 +1676,22 @@ static int osc_makes_rpc(struct client_obd *cli, struct osc_object *osc,
 	 * before canceling the locks and evicting down the llite pages
 	 */
 	if (!cli->cl_import || cli->cl_import->imp_invalid)
-		invalid_import = 1;
+		invalid_import = true;
 
 	if (cmd & OBD_BRW_WRITE) {
 		if (atomic_read(&osc->oo_nr_writes) == 0)
-			return 0;
+			return false;
 		if (invalid_import) {
 			CDEBUG(D_CACHE, "invalid import forcing RPC\n");
-			return 1;
+			return true;
 		}
 		if (!list_empty(&osc->oo_hp_exts)) {
 			CDEBUG(D_CACHE, "high prio request forcing RPC\n");
-			return 1;
+			return true;
 		}
 		if (!list_empty(&osc->oo_urgent_exts)) {
 			CDEBUG(D_CACHE, "urgent request forcing RPC\n");
-			return 1;
+			return true;
 		}
 		/* trigger a write rpc stream as long as there are dirtiers
 		 * waiting for space.  as they're waiting, they're not going to
@@ -1699,25 +1699,25 @@ static int osc_makes_rpc(struct client_obd *cli, struct osc_object *osc,
 		 */
 		if (waitqueue_active(&cli->cl_cache_waiters)) {
 			CDEBUG(D_CACHE, "cache waiters forcing RPC\n");
-			return 1;
+			return true;
 		}
 		if (!list_empty(&osc->oo_full_exts)) {
 			CDEBUG(D_CACHE, "full extent ready, make an RPC\n");
-			return 1;
+			return true;
 		}
 	} else {
 		if (atomic_read(&osc->oo_nr_reads) == 0)
-			return 0;
+			return false;
 		if (invalid_import) {
 			CDEBUG(D_CACHE, "invalid import forcing RPC\n");
-			return 1;
+			return true;
 		}
 		/* all read are urgent. */
 		if (!list_empty(&osc->oo_reading_exts))
-			return 1;
+			return true;
 	}
 
-	return 0;
+	return false;
 }
 
 static void osc_update_pending(struct osc_object *obj, int cmd, int delta)

[lustre-devel] [PATCH 25/29] lustre: remove cl_page_cancel()

[NeilBrown]

cl_page_cancel() is never used, so remove it and various
other things that it is the only user of.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 drivers/staging/lustre/lustre/include/cl_object.h  |   18 -----
 drivers/staging/lustre/lustre/include/lustre_net.h |    1 
 drivers/staging/lustre/lustre/obdclass/cl_page.c   |   20 ------
 drivers/staging/lustre/lustre/osc/osc_cache.c      |   67 --------------------
 .../staging/lustre/lustre/osc/osc_cl_internal.h    |    1 
 drivers/staging/lustre/lustre/osc/osc_internal.h   |    1 
 drivers/staging/lustre/lustre/osc/osc_page.c       |   20 ------
 drivers/staging/lustre/lustre/osc/osc_request.c    |    9 ---
 drivers/staging/lustre/lustre/ptlrpc/client.c      |   15 +---
 9 files changed, 4 insertions(+), 148 deletions(-)

diff --git a/drivers/staging/lustre/lustre/include/cl_object.h b/drivers/staging/lustre/lustre/include/cl_object.h
index a1e07f8b5eda..de5d68879740 100644
--- a/drivers/staging/lustre/lustre/include/cl_object.h
+++ b/drivers/staging/lustre/lustre/include/cl_object.h
@@ -969,23 +969,6 @@ struct cl_page_operations {
 	void (*cpo_clip)(const struct lu_env *env,
 			 const struct cl_page_slice *slice,
 			 int from, int to);
-	/**
-	 * \pre  the page was queued for transferring.
-	 * \post page is removed from client's pending list, or -EBUSY
-	 *       is returned if it has already been in transferring.
-	 *
-	 * This is one of seldom page operation which is:
-	 * 0. called from top level;
-	 * 1. don't have vmpage locked;
-	 * 2. every layer should synchronize execution of its ->cpo_cancel()
-	 *    with completion handlers. Osc uses client obd lock for this
-	 *    purpose. Based on there is no vvp_page_cancel and
-	 *    lov_page_cancel(), cpo_cancel is defacto protected by client lock.
-	 *
-	 * \see osc_page_cancel().
-	 */
-	int (*cpo_cancel)(const struct lu_env *env,
-			  const struct cl_page_slice *slice);
 	/**
 	 * Write out a page by kernel. This is only called by ll_writepage
 	 * right now.
@@ -2159,7 +2142,6 @@ int cl_page_cache_add(const struct lu_env *env, struct cl_io *io,
 		      struct cl_page *pg, enum cl_req_type crt);
 void cl_page_clip(const struct lu_env *env, struct cl_page *pg,
 		  int from, int to);
-int cl_page_cancel(const struct lu_env *env, struct cl_page *page);
 int cl_page_flush(const struct lu_env *env, struct cl_io *io,
 		  struct cl_page *pg);
 
diff --git a/drivers/staging/lustre/lustre/include/lustre_net.h b/drivers/staging/lustre/lustre/include/lustre_net.h
index 468a03edefd9..6d328b48a96b 100644
--- a/drivers/staging/lustre/lustre/include/lustre_net.h
+++ b/drivers/staging/lustre/lustre/include/lustre_net.h
@@ -1830,7 +1830,6 @@ struct ptlrpc_request_set *ptlrpc_prep_fcset(int max, set_producer_func func,
 					     void *arg);
 int ptlrpc_check_set(const struct lu_env *env, struct ptlrpc_request_set *set);
 int ptlrpc_set_wait(struct ptlrpc_request_set *);
-void ptlrpc_mark_interrupted(struct ptlrpc_request *req);
 void ptlrpc_set_destroy(struct ptlrpc_request_set *);
 void ptlrpc_set_add_req(struct ptlrpc_request_set *, struct ptlrpc_request *);
 
diff --git a/drivers/staging/lustre/lustre/obdclass/cl_page.c b/drivers/staging/lustre/lustre/obdclass/cl_page.c
index 00df94b87606..217a5ebe1691 100644
--- a/drivers/staging/lustre/lustre/obdclass/cl_page.c
+++ b/drivers/staging/lustre/lustre/obdclass/cl_page.c
@@ -932,26 +932,6 @@ void cl_page_print(const struct lu_env *env, void *cookie,
 }
 EXPORT_SYMBOL(cl_page_print);
 
-/**
- * Cancel a page which is still in a transfer.
- */
-int cl_page_cancel(const struct lu_env *env, struct cl_page *page)
-{
-	const struct cl_page_slice *slice;
-	int result = 0;
-
-	list_for_each_entry(slice, &page->cp_layers, cpl_linkage) {
-		if (slice->cpl_ops->cpo_cancel)
-			result = (*slice->cpl_ops->cpo_cancel)(env, slice);
-		if (result != 0)
-			break;
-	}
-	if (result > 0)
-		result = 0;
-
-	return result;
-}
-
 /**
  * Converts a byte offset within object \a obj into a page index.
  */
diff --git a/drivers/staging/lustre/lustre/osc/osc_cache.c b/drivers/staging/lustre/lustre/osc/osc_cache.c
index 1476f84e6156..79bcaa212339 100644
--- a/drivers/staging/lustre/lustre/osc/osc_cache.c
+++ b/drivers/staging/lustre/lustre/osc/osc_cache.c
@@ -1822,7 +1822,6 @@ static void osc_ap_completion(const struct lu_env *env, struct client_obd *cli,
 	spin_lock(&oap->oap_lock);
 	oap->oap_async_flags = 0;
 	spin_unlock(&oap->oap_lock);
-	oap->oap_interrupted = 0;
 
 	if (oap->oap_cmd & OBD_BRW_WRITE && xid > 0) {
 		spin_lock(&cli->cl_loi_list_lock);
@@ -2591,72 +2590,6 @@ int osc_flush_async_page(const struct lu_env *env, struct cl_io *io,
 	return rc;
 }
 
-/**
- * this is called when a sync waiter receives an interruption.  Its job is to
- * get the caller woken as soon as possible.  If its page hasn't been put in an
- * rpc yet it can dequeue immediately.  Otherwise it has to mark the rpc as
- * desiring interruption which will forcefully complete the rpc once the rpc
- * has timed out.
- */
-int osc_cancel_async_page(const struct lu_env *env, struct osc_page *ops)
-{
-	struct osc_async_page *oap = &ops->ops_oap;
-	struct osc_object *obj = oap->oap_obj;
-	struct client_obd *cli = osc_cli(obj);
-	struct osc_extent *ext;
-	struct osc_extent *found = NULL;
-	struct list_head *plist;
-	pgoff_t index = osc_index(ops);
-	int rc = -EBUSY;
-	int cmd;
-
-	LASSERT(!oap->oap_interrupted);
-	oap->oap_interrupted = 1;
-
-	/* Find out the caching extent */
-	osc_object_lock(obj);
-	if (oap->oap_cmd & OBD_BRW_WRITE) {
-		plist = &obj->oo_urgent_exts;
-		cmd = OBD_BRW_WRITE;
-	} else {
-		plist = &obj->oo_reading_exts;
-		cmd = OBD_BRW_READ;
-	}
-	list_for_each_entry(ext, plist, oe_link) {
-		if (ext->oe_start <= index && ext->oe_end >= index) {
-			LASSERT(ext->oe_state == OES_LOCK_DONE);
-			/* For OES_LOCK_DONE state extent, it has already held
-			 * a refcount for RPC.
-			 */
-			found = osc_extent_get(ext);
-			break;
-		}
-	}
-	if (found) {
-		list_del_init(&found->oe_link);
-		osc_update_pending(obj, cmd, -found->oe_nr_pages);
-		osc_object_unlock(obj);
-
-		osc_extent_finish(env, found, 0, -EINTR);
-		osc_extent_put(env, found);
-		rc = 0;
-	} else {
-		osc_object_unlock(obj);
-		/* ok, it's been put in an rpc. only one oap gets a request
-		 * reference
-		 */
-		if (oap->oap_request) {
-			ptlrpc_mark_interrupted(oap->oap_request);
-			ptlrpcd_wake(oap->oap_request);
-			ptlrpc_req_finished(oap->oap_request);
-			oap->oap_request = NULL;
-		}
-	}
-
-	osc_list_maint(cli, obj);
-	return rc;
-}
-
 int osc_queue_sync_pages(const struct lu_env *env, struct osc_object *obj,
 			 struct list_head *list, int cmd, int brw_flags)
 {
diff --git a/drivers/staging/lustre/lustre/osc/osc_cl_internal.h b/drivers/staging/lustre/lustre/osc/osc_cl_internal.h
index b1a1d241cc6c..3af096e0dbdd 100644
--- a/drivers/staging/lustre/lustre/osc/osc_cl_internal.h
+++ b/drivers/staging/lustre/lustre/osc/osc_cl_internal.h
@@ -380,7 +380,6 @@ int osc_lvb_print(const struct lu_env *env, void *cookie,
 void osc_lru_add_batch(struct client_obd *cli, struct list_head *list);
 void osc_page_submit(const struct lu_env *env, struct osc_page *opg,
 		     enum cl_req_type crt, int brw_flags);
-int osc_cancel_async_page(const struct lu_env *env, struct osc_page *ops);
 int osc_set_async_flags(struct osc_object *obj, struct osc_page *opg,
 			u32 async_flags);
 int osc_prep_async_page(struct osc_object *osc, struct osc_page *ops,
diff --git a/drivers/staging/lustre/lustre/osc/osc_internal.h b/drivers/staging/lustre/lustre/osc/osc_internal.h
index 0354272fe192..586f0dfe3790 100644
--- a/drivers/staging/lustre/lustre/osc/osc_internal.h
+++ b/drivers/staging/lustre/lustre/osc/osc_internal.h
@@ -60,7 +60,6 @@ enum async_flags {
 struct osc_async_page {
 	int				oap_magic;
 	unsigned short			oap_cmd;
-	unsigned short			oap_interrupted:1;
 
 	struct list_head		oap_pending_item;
 	struct list_head		oap_rpc_item;
diff --git a/drivers/staging/lustre/lustre/osc/osc_page.c b/drivers/staging/lustre/lustre/osc/osc_page.c
index 28b12729d7e9..e0187fafcc37 100644
--- a/drivers/staging/lustre/lustre/osc/osc_page.c
+++ b/drivers/staging/lustre/lustre/osc/osc_page.c
@@ -137,11 +137,10 @@ static int osc_page_print(const struct lu_env *env,
 	struct osc_object *obj = cl2osc(slice->cpl_obj);
 	struct client_obd *cli = &osc_export(obj)->exp_obd->u.cli;
 
-	return (*printer)(env, cookie, LUSTRE_OSC_NAME "-page@%p %lu: 1< %#x %d %u %s %s > 2< %llu %u %u %#x %#x | %p %p %p > 3< %d %lu %d > 4< %d %d %d %lu %s | %s %s %s %s > 5< %s %s %s %s | %d %s | %d %s %s>\n",
+	return (*printer)(env, cookie, LUSTRE_OSC_NAME "-page@%p %lu: 1< %#x %d %s %s > 2< %llu %u %u %#x %#x | %p %p %p > 3< %d %lu %d > 4< %d %d %d %lu %s | %s %s %s %s > 5< %s %s %s %s | %d %s | %d %s %s>\n",
 			  opg, osc_index(opg),
 			  /* 1 */
 			  oap->oap_magic, oap->oap_cmd,
-			  oap->oap_interrupted,
 			  osc_list(&oap->oap_pending_item),
 			  osc_list(&oap->oap_rpc_item),
 			  /* 2 */
@@ -216,22 +215,6 @@ static void osc_page_clip(const struct lu_env *env,
 	spin_unlock(&oap->oap_lock);
 }
 
-static int osc_page_cancel(const struct lu_env *env,
-			   const struct cl_page_slice *slice)
-{
-	struct osc_page *opg = cl2osc_page(slice);
-	int rc = 0;
-
-	/* Check if the transferring against this page
-	 * is completed, or not even queued.
-	 */
-	if (opg->ops_transfer_pinned)
-		/* FIXME: may not be interrupted.. */
-		rc = osc_cancel_async_page(env, opg);
-	LASSERT(ergo(rc == 0, opg->ops_transfer_pinned == 0));
-	return rc;
-}
-
 static int osc_page_flush(const struct lu_env *env,
 			  const struct cl_page_slice *slice,
 			  struct cl_io *io)
@@ -247,7 +230,6 @@ static const struct cl_page_operations osc_page_ops = {
 	.cpo_print	 = osc_page_print,
 	.cpo_delete	= osc_page_delete,
 	.cpo_clip	   = osc_page_clip,
-	.cpo_cancel	 = osc_page_cancel,
 	.cpo_flush	  = osc_page_flush
 };
 
diff --git a/drivers/staging/lustre/lustre/osc/osc_request.c b/drivers/staging/lustre/lustre/osc/osc_request.c
index b28fbacbcfbf..ccc491efa982 100644
--- a/drivers/staging/lustre/lustre/osc/osc_request.c
+++ b/drivers/staging/lustre/lustre/osc/osc_request.c
@@ -1635,10 +1635,6 @@ static int osc_brw_redo_request(struct ptlrpc_request *request,
 			LASSERTF(request == oap->oap_request,
 				 "request %p != oap_request %p\n",
 				 request, oap->oap_request);
-			if (oap->oap_interrupted) {
-				ptlrpc_req_finished(new_req);
-				return -EINTR;
-			}
 		}
 	}
 	/* New request takes over pga and oaps from old request.
@@ -1879,7 +1875,6 @@ int osc_build_rpc(const struct lu_env *env, struct client_obd *cli,
 	int mem_tight = 0;
 	int page_count = 0;
 	bool soft_sync = false;
-	bool interrupted = false;
 	int grant = 0;
 	int i;
 	int rc;
@@ -1937,8 +1932,6 @@ int osc_build_rpc(const struct lu_env *env, struct client_obd *cli,
 			else
 				LASSERT(oap->oap_page_off + oap->oap_count ==
 					PAGE_SIZE);
-			if (oap->oap_interrupted)
-				interrupted = true;
 		}
 	}
 
@@ -1968,8 +1961,6 @@ int osc_build_rpc(const struct lu_env *env, struct client_obd *cli,
 
 	req->rq_memalloc = mem_tight != 0;
 	oap->oap_request = ptlrpc_request_addref(req);
-	if (interrupted && !req->rq_intr)
-		ptlrpc_mark_interrupted(req);
 
 	/* Need to update the timestamps after the request is built in case
 	 * we race with setattr (locally or in queue at OST).  If OST gets
diff --git a/drivers/staging/lustre/lustre/ptlrpc/client.c b/drivers/staging/lustre/lustre/ptlrpc/client.c
index 8fafc8dc3f57..f90a3eef5daf 100644
--- a/drivers/staging/lustre/lustre/ptlrpc/client.c
+++ b/drivers/staging/lustre/lustre/ptlrpc/client.c
@@ -2148,17 +2148,6 @@ void ptlrpc_expired_set(struct ptlrpc_request_set *set)
 	}
 }
 
-/**
- * Sets rq_intr flag in \a req under spinlock.
- */
-void ptlrpc_mark_interrupted(struct ptlrpc_request *req)
-{
-	spin_lock(&req->rq_lock);
-	req->rq_intr = 1;
-	spin_unlock(&req->rq_lock);
-}
-EXPORT_SYMBOL(ptlrpc_mark_interrupted);
-
 /**
  * Interrupts (sets interrupted flag) all uncompleted requests in
  * a set \a data. Called when l_wait_event_abortable_timeout receives signal.
@@ -2174,7 +2163,9 @@ static void ptlrpc_interrupted_set(struct ptlrpc_request_set *set)
 		    req->rq_phase != RQ_PHASE_UNREG_RPC)
 			continue;
 
-		ptlrpc_mark_interrupted(req);
+		spin_lock(&req->rq_lock);
+		req->rq_intr = 1;
+		spin_unlock(&req->rq_lock);
 	}
 }
 

[lustre-devel] [PATCH 25/29] lustre: remove cl_page_cancel()

[Andreas Dilger]

On Jan 8, 2019, at 23:24, NeilBrown <neilb@suse.com> wrote:
> 
> cl_page_cancel() is never used, so remove it and various
> other things that it is the only user of.
> 
> Signed-off-by: NeilBrown <neilb@suse.com>

Looks good.  Nice to see so much dead code being removed.

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

> ---
> drivers/staging/lustre/lustre/include/cl_object.h  |   18 -----
> drivers/staging/lustre/lustre/include/lustre_net.h |    1 
> drivers/staging/lustre/lustre/obdclass/cl_page.c   |   20 ------
> drivers/staging/lustre/lustre/osc/osc_cache.c      |   67 --------------------
> .../staging/lustre/lustre/osc/osc_cl_internal.h    |    1 
> drivers/staging/lustre/lustre/osc/osc_internal.h   |    1 
> drivers/staging/lustre/lustre/osc/osc_page.c       |   20 ------
> drivers/staging/lustre/lustre/osc/osc_request.c    |    9 ---
> drivers/staging/lustre/lustre/ptlrpc/client.c      |   15 +---
> 9 files changed, 4 insertions(+), 148 deletions(-)
> 
> diff --git a/drivers/staging/lustre/lustre/include/cl_object.h b/drivers/staging/lustre/lustre/include/cl_object.h
> index a1e07f8b5eda..de5d68879740 100644
> --- a/drivers/staging/lustre/lustre/include/cl_object.h
> +++ b/drivers/staging/lustre/lustre/include/cl_object.h
> @@ -969,23 +969,6 @@ struct cl_page_operations {
> 	void (*cpo_clip)(const struct lu_env *env,
> 			 const struct cl_page_slice *slice,
> 			 int from, int to);
> -	/**
> -	 * \pre  the page was queued for transferring.
> -	 * \post page is removed from client's pending list, or -EBUSY
> -	 *       is returned if it has already been in transferring.
> -	 *
> -	 * This is one of seldom page operation which is:
> -	 * 0. called from top level;
> -	 * 1. don't have vmpage locked;
> -	 * 2. every layer should synchronize execution of its ->cpo_cancel()
> -	 *    with completion handlers. Osc uses client obd lock for this
> -	 *    purpose. Based on there is no vvp_page_cancel and
> -	 *    lov_page_cancel(), cpo_cancel is defacto protected by client lock.
> -	 *
> -	 * \see osc_page_cancel().
> -	 */
> -	int (*cpo_cancel)(const struct lu_env *env,
> -			  const struct cl_page_slice *slice);
> 	/**
> 	 * Write out a page by kernel. This is only called by ll_writepage
> 	 * right now.
> @@ -2159,7 +2142,6 @@ int cl_page_cache_add(const struct lu_env *env, struct cl_io *io,
> 		      struct cl_page *pg, enum cl_req_type crt);
> void cl_page_clip(const struct lu_env *env, struct cl_page *pg,
> 		  int from, int to);
> -int cl_page_cancel(const struct lu_env *env, struct cl_page *page);
> int cl_page_flush(const struct lu_env *env, struct cl_io *io,
> 		  struct cl_page *pg);
> 
> diff --git a/drivers/staging/lustre/lustre/include/lustre_net.h b/drivers/staging/lustre/lustre/include/lustre_net.h
> index 468a03edefd9..6d328b48a96b 100644
> --- a/drivers/staging/lustre/lustre/include/lustre_net.h
> +++ b/drivers/staging/lustre/lustre/include/lustre_net.h
> @@ -1830,7 +1830,6 @@ struct ptlrpc_request_set *ptlrpc_prep_fcset(int max, set_producer_func func,
> 					     void *arg);
> int ptlrpc_check_set(const struct lu_env *env, struct ptlrpc_request_set *set);
> int ptlrpc_set_wait(struct ptlrpc_request_set *);
> -void ptlrpc_mark_interrupted(struct ptlrpc_request *req);
> void ptlrpc_set_destroy(struct ptlrpc_request_set *);
> void ptlrpc_set_add_req(struct ptlrpc_request_set *, struct ptlrpc_request *);
> 
> diff --git a/drivers/staging/lustre/lustre/obdclass/cl_page.c b/drivers/staging/lustre/lustre/obdclass/cl_page.c
> index 00df94b87606..217a5ebe1691 100644
> --- a/drivers/staging/lustre/lustre/obdclass/cl_page.c
> +++ b/drivers/staging/lustre/lustre/obdclass/cl_page.c
> @@ -932,26 +932,6 @@ void cl_page_print(const struct lu_env *env, void *cookie,
> }
> EXPORT_SYMBOL(cl_page_print);
> 
> -/**
> - * Cancel a page which is still in a transfer.
> - */
> -int cl_page_cancel(const struct lu_env *env, struct cl_page *page)
> -{
> -	const struct cl_page_slice *slice;
> -	int result = 0;
> -
> -	list_for_each_entry(slice, &page->cp_layers, cpl_linkage) {
> -		if (slice->cpl_ops->cpo_cancel)
> -			result = (*slice->cpl_ops->cpo_cancel)(env, slice);
> -		if (result != 0)
> -			break;
> -	}
> -	if (result > 0)
> -		result = 0;
> -
> -	return result;
> -}
> -
> /**
>  * Converts a byte offset within object \a obj into a page index.
>  */
> diff --git a/drivers/staging/lustre/lustre/osc/osc_cache.c b/drivers/staging/lustre/lustre/osc/osc_cache.c
> index 1476f84e6156..79bcaa212339 100644
> --- a/drivers/staging/lustre/lustre/osc/osc_cache.c
> +++ b/drivers/staging/lustre/lustre/osc/osc_cache.c
> @@ -1822,7 +1822,6 @@ static void osc_ap_completion(const struct lu_env *env, struct client_obd *cli,
> 	spin_lock(&oap->oap_lock);
> 	oap->oap_async_flags = 0;
> 	spin_unlock(&oap->oap_lock);
> -	oap->oap_interrupted = 0;
> 
> 	if (oap->oap_cmd & OBD_BRW_WRITE && xid > 0) {
> 		spin_lock(&cli->cl_loi_list_lock);
> @@ -2591,72 +2590,6 @@ int osc_flush_async_page(const struct lu_env *env, struct cl_io *io,
> 	return rc;
> }
> 
> -/**
> - * this is called when a sync waiter receives an interruption.  Its job is to
> - * get the caller woken as soon as possible.  If its page hasn't been put in an
> - * rpc yet it can dequeue immediately.  Otherwise it has to mark the rpc as
> - * desiring interruption which will forcefully complete the rpc once the rpc
> - * has timed out.
> - */
> -int osc_cancel_async_page(const struct lu_env *env, struct osc_page *ops)
> -{
> -	struct osc_async_page *oap = &ops->ops_oap;
> -	struct osc_object *obj = oap->oap_obj;
> -	struct client_obd *cli = osc_cli(obj);
> -	struct osc_extent *ext;
> -	struct osc_extent *found = NULL;
> -	struct list_head *plist;
> -	pgoff_t index = osc_index(ops);
> -	int rc = -EBUSY;
> -	int cmd;
> -
> -	LASSERT(!oap->oap_interrupted);
> -	oap->oap_interrupted = 1;
> -
> -	/* Find out the caching extent */
> -	osc_object_lock(obj);
> -	if (oap->oap_cmd & OBD_BRW_WRITE) {
> -		plist = &obj->oo_urgent_exts;
> -		cmd = OBD_BRW_WRITE;
> -	} else {
> -		plist = &obj->oo_reading_exts;
> -		cmd = OBD_BRW_READ;
> -	}
> -	list_for_each_entry(ext, plist, oe_link) {
> -		if (ext->oe_start <= index && ext->oe_end >= index) {
> -			LASSERT(ext->oe_state == OES_LOCK_DONE);
> -			/* For OES_LOCK_DONE state extent, it has already held
> -			 * a refcount for RPC.
> -			 */
> -			found = osc_extent_get(ext);
> -			break;
> -		}
> -	}
> -	if (found) {
> -		list_del_init(&found->oe_link);
> -		osc_update_pending(obj, cmd, -found->oe_nr_pages);
> -		osc_object_unlock(obj);
> -
> -		osc_extent_finish(env, found, 0, -EINTR);
> -		osc_extent_put(env, found);
> -		rc = 0;
> -	} else {
> -		osc_object_unlock(obj);
> -		/* ok, it's been put in an rpc. only one oap gets a request
> -		 * reference
> -		 */
> -		if (oap->oap_request) {
> -			ptlrpc_mark_interrupted(oap->oap_request);
> -			ptlrpcd_wake(oap->oap_request);
> -			ptlrpc_req_finished(oap->oap_request);
> -			oap->oap_request = NULL;
> -		}
> -	}
> -
> -	osc_list_maint(cli, obj);
> -	return rc;
> -}
> -
> int osc_queue_sync_pages(const struct lu_env *env, struct osc_object *obj,
> 			 struct list_head *list, int cmd, int brw_flags)
> {
> diff --git a/drivers/staging/lustre/lustre/osc/osc_cl_internal.h b/drivers/staging/lustre/lustre/osc/osc_cl_internal.h
> index b1a1d241cc6c..3af096e0dbdd 100644
> --- a/drivers/staging/lustre/lustre/osc/osc_cl_internal.h
> +++ b/drivers/staging/lustre/lustre/osc/osc_cl_internal.h
> @@ -380,7 +380,6 @@ int osc_lvb_print(const struct lu_env *env, void *cookie,
> void osc_lru_add_batch(struct client_obd *cli, struct list_head *list);
> void osc_page_submit(const struct lu_env *env, struct osc_page *opg,
> 		     enum cl_req_type crt, int brw_flags);
> -int osc_cancel_async_page(const struct lu_env *env, struct osc_page *ops);
> int osc_set_async_flags(struct osc_object *obj, struct osc_page *opg,
> 			u32 async_flags);
> int osc_prep_async_page(struct osc_object *osc, struct osc_page *ops,
> diff --git a/drivers/staging/lustre/lustre/osc/osc_internal.h b/drivers/staging/lustre/lustre/osc/osc_internal.h
> index 0354272fe192..586f0dfe3790 100644
> --- a/drivers/staging/lustre/lustre/osc/osc_internal.h
> +++ b/drivers/staging/lustre/lustre/osc/osc_internal.h
> @@ -60,7 +60,6 @@ enum async_flags {
> struct osc_async_page {
> 	int				oap_magic;
> 	unsigned short			oap_cmd;
> -	unsigned short			oap_interrupted:1;
> 
> 	struct list_head		oap_pending_item;
> 	struct list_head		oap_rpc_item;
> diff --git a/drivers/staging/lustre/lustre/osc/osc_page.c b/drivers/staging/lustre/lustre/osc/osc_page.c
> index 28b12729d7e9..e0187fafcc37 100644
> --- a/drivers/staging/lustre/lustre/osc/osc_page.c
> +++ b/drivers/staging/lustre/lustre/osc/osc_page.c
> @@ -137,11 +137,10 @@ static int osc_page_print(const struct lu_env *env,
> 	struct osc_object *obj = cl2osc(slice->cpl_obj);
> 	struct client_obd *cli = &osc_export(obj)->exp_obd->u.cli;
> 
> -	return (*printer)(env, cookie, LUSTRE_OSC_NAME "-page@%p %lu: 1< %#x %d %u %s %s > 2< %llu %u %u %#x %#x | %p %p %p > 3< %d %lu %d > 4< %d %d %d %lu %s | %s %s %s %s > 5< %s %s %s %s | %d %s | %d %s %s>\n",
> +	return (*printer)(env, cookie, LUSTRE_OSC_NAME "-page@%p %lu: 1< %#x %d %s %s > 2< %llu %u %u %#x %#x | %p %p %p > 3< %d %lu %d > 4< %d %d %d %lu %s | %s %s %s %s > 5< %s %s %s %s | %d %s | %d %s %s>\n",
> 			  opg, osc_index(opg),
> 			  /* 1 */
> 			  oap->oap_magic, oap->oap_cmd,
> -			  oap->oap_interrupted,
> 			  osc_list(&oap->oap_pending_item),
> 			  osc_list(&oap->oap_rpc_item),
> 			  /* 2 */
> @@ -216,22 +215,6 @@ static void osc_page_clip(const struct lu_env *env,
> 	spin_unlock(&oap->oap_lock);
> }
> 
> -static int osc_page_cancel(const struct lu_env *env,
> -			   const struct cl_page_slice *slice)
> -{
> -	struct osc_page *opg = cl2osc_page(slice);
> -	int rc = 0;
> -
> -	/* Check if the transferring against this page
> -	 * is completed, or not even queued.
> -	 */
> -	if (opg->ops_transfer_pinned)
> -		/* FIXME: may not be interrupted.. */
> -		rc = osc_cancel_async_page(env, opg);
> -	LASSERT(ergo(rc == 0, opg->ops_transfer_pinned == 0));
> -	return rc;
> -}
> -
> static int osc_page_flush(const struct lu_env *env,
> 			  const struct cl_page_slice *slice,
> 			  struct cl_io *io)
> @@ -247,7 +230,6 @@ static const struct cl_page_operations osc_page_ops = {
> 	.cpo_print	 = osc_page_print,
> 	.cpo_delete	= osc_page_delete,
> 	.cpo_clip	   = osc_page_clip,
> -	.cpo_cancel	 = osc_page_cancel,
> 	.cpo_flush	  = osc_page_flush
> };
> 
> diff --git a/drivers/staging/lustre/lustre/osc/osc_request.c b/drivers/staging/lustre/lustre/osc/osc_request.c
> index b28fbacbcfbf..ccc491efa982 100644
> --- a/drivers/staging/lustre/lustre/osc/osc_request.c
> +++ b/drivers/staging/lustre/lustre/osc/osc_request.c
> @@ -1635,10 +1635,6 @@ static int osc_brw_redo_request(struct ptlrpc_request *request,
> 			LASSERTF(request == oap->oap_request,
> 				 "request %p != oap_request %p\n",
> 				 request, oap->oap_request);
> -			if (oap->oap_interrupted) {
> -				ptlrpc_req_finished(new_req);
> -				return -EINTR;
> -			}
> 		}
> 	}
> 	/* New request takes over pga and oaps from old request.
> @@ -1879,7 +1875,6 @@ int osc_build_rpc(const struct lu_env *env, struct client_obd *cli,
> 	int mem_tight = 0;
> 	int page_count = 0;
> 	bool soft_sync = false;
> -	bool interrupted = false;
> 	int grant = 0;
> 	int i;
> 	int rc;
> @@ -1937,8 +1932,6 @@ int osc_build_rpc(const struct lu_env *env, struct client_obd *cli,
> 			else
> 				LASSERT(oap->oap_page_off + oap->oap_count ==
> 					PAGE_SIZE);
> -			if (oap->oap_interrupted)
> -				interrupted = true;
> 		}
> 	}
> 
> @@ -1968,8 +1961,6 @@ int osc_build_rpc(const struct lu_env *env, struct client_obd *cli,
> 
> 	req->rq_memalloc = mem_tight != 0;
> 	oap->oap_request = ptlrpc_request_addref(req);
> -	if (interrupted && !req->rq_intr)
> -		ptlrpc_mark_interrupted(req);
> 
> 	/* Need to update the timestamps after the request is built in case
> 	 * we race with setattr (locally or in queue at OST).  If OST gets
> diff --git a/drivers/staging/lustre/lustre/ptlrpc/client.c b/drivers/staging/lustre/lustre/ptlrpc/client.c
> index 8fafc8dc3f57..f90a3eef5daf 100644
> --- a/drivers/staging/lustre/lustre/ptlrpc/client.c
> +++ b/drivers/staging/lustre/lustre/ptlrpc/client.c
> @@ -2148,17 +2148,6 @@ void ptlrpc_expired_set(struct ptlrpc_request_set *set)
> 	}
> }
> 
> -/**
> - * Sets rq_intr flag in \a req under spinlock.
> - */
> -void ptlrpc_mark_interrupted(struct ptlrpc_request *req)
> -{
> -	spin_lock(&req->rq_lock);
> -	req->rq_intr = 1;
> -	spin_unlock(&req->rq_lock);
> -}
> -EXPORT_SYMBOL(ptlrpc_mark_interrupted);
> -
> /**
>  * Interrupts (sets interrupted flag) all uncompleted requests in
>  * a set \a data. Called when l_wait_event_abortable_timeout receives signal.
> @@ -2174,7 +2163,9 @@ static void ptlrpc_interrupted_set(struct ptlrpc_request_set *set)
> 		    req->rq_phase != RQ_PHASE_UNREG_RPC)
> 			continue;
> 
> -		ptlrpc_mark_interrupted(req);
> +		spin_lock(&req->rq_lock);
> +		req->rq_intr = 1;
> +		spin_unlock(&req->rq_lock);
> 	}
> }
> 
> 
> 

Cheers, Andreas
---
Andreas Dilger
CTO Whamcloud

[lustre-devel] [PATCH 22/29] lustre: osc_cache: osc_prep_async_page() has meaningless return

[NeilBrown]

osc_prep_async_page() is only called by osc_page_init(),
which immediately returns any non-zero return value.
osc_page_init is a ->coo_page_init() function, only called
by cl_page_alloc(), which treats any non-zero return as a
negative error number.

So osc_prep_async_page() must return 0 or a negative error.
Currently it can return
  cfs_size_round(sizeof(*oap))
which is either of those.

Presumably this condition never happens, so the return value is
irrelevant.
We could make it an LASSERT(), but it is safer to just return
an generic error.  So make it
  return -EIO;

Signed-off-by: NeilBrown <neilb@suse.com>
---
 drivers/staging/lustre/lustre/osc/osc_cache.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/lustre/lustre/osc/osc_cache.c b/drivers/staging/lustre/lustre/osc/osc_cache.c
index b2ad6a15014e..8a68d3eb9314 100644
--- a/drivers/staging/lustre/lustre/osc/osc_cache.c
+++ b/drivers/staging/lustre/lustre/osc/osc_cache.c
@@ -2287,7 +2287,7 @@ int osc_prep_async_page(struct osc_object *osc, struct osc_page *ops,
 	struct osc_async_page *oap = &ops->ops_oap;
 
 	if (!page)
-		return cfs_size_round(sizeof(*oap));
+		return -EIO;
 
 	oap->oap_magic = OAP_MAGIC;
 	oap->oap_cli = &exp->exp_obd->u.cli;

[lustre-devel] [PATCH 14/29] lustre: osc_cache: convert cl_cache_waiters to a wait_queue.

[NeilBrown]

cli->cl_cache_waiters is a list of tasks that need
to be woken when grant-space becomes available.  This
means it is acting much like a wait queue.
So let's change it to really be a wait queue.

The current implementation adds new waiters to the end of the list,
and calls osc_enter_cache_try() on each in order.
We can provide the same behaviour by using an exclusive wait,
and having each waiter wake the next task when it succeeds.

If a waiter notices that success has become impossible, it wakes all
other waiters.

If a waiter times out, it doesn't wake other - just leaves them to
time out themselves.

Note that the old code handled -EINTR from the wait function.  That is
not a possible return value when wait_event_idle* is used, so that
case is discarded.

For all this to work, we need a
  wait_event_idle_exclusive_timeout_cmd()
macro. This fits the pattern of other macros in wait.h, and can
be moved to wait.h when this code lands in mainline.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 drivers/staging/lustre/lustre/include/obd.h      |    2 
 drivers/staging/lustre/lustre/ldlm/ldlm_lib.c    |    2 
 drivers/staging/lustre/lustre/osc/osc_cache.c    |  145 ++++++++--------------
 drivers/staging/lustre/lustre/osc/osc_internal.h |   12 +-
 drivers/staging/lustre/lustre/osc/osc_page.c     |    2 
 5 files changed, 57 insertions(+), 106 deletions(-)

diff --git a/drivers/staging/lustre/lustre/include/obd.h b/drivers/staging/lustre/lustre/include/obd.h
index d6a968ceb274..bb6f3e1fce6e 100644
--- a/drivers/staging/lustre/lustre/include/obd.h
+++ b/drivers/staging/lustre/lustre/include/obd.h
@@ -209,7 +209,7 @@ struct client_obd {
 	 * See osc_{reserve|unreserve}_grant for details.
 	 */
 	long		 cl_reserved_grant;
-	struct list_head cl_cache_waiters; /* waiting for cache/grant */
+	wait_queue_head_t cl_cache_waiters; /* waiting for cache/grant */
 	unsigned long	 cl_next_shrink_grant;   /* jiffies */
 	struct list_head cl_grant_shrink_list;  /* Timeout event list */
 	int		 cl_grant_shrink_interval; /* seconds */
diff --git a/drivers/staging/lustre/lustre/ldlm/ldlm_lib.c b/drivers/staging/lustre/lustre/ldlm/ldlm_lib.c
index 732ef3a64c72..609b9d04eb40 100644
--- a/drivers/staging/lustre/lustre/ldlm/ldlm_lib.c
+++ b/drivers/staging/lustre/lustre/ldlm/ldlm_lib.c
@@ -323,7 +323,7 @@ int client_obd_setup(struct obd_device *obddev, struct lustre_cfg *lcfg)
 	 * ptlrpc_connect_interpret().
 	 */
 	client_adjust_max_dirty(cli);
-	INIT_LIST_HEAD(&cli->cl_cache_waiters);
+	init_waitqueue_head(&cli->cl_cache_waiters);
 	INIT_LIST_HEAD(&cli->cl_loi_ready_list);
 	INIT_LIST_HEAD(&cli->cl_loi_hp_ready_list);
 	INIT_LIST_HEAD(&cli->cl_loi_write_list);
diff --git a/drivers/staging/lustre/lustre/osc/osc_cache.c b/drivers/staging/lustre/lustre/osc/osc_cache.c
index e9987c187ecd..ddfb61502f30 100644
--- a/drivers/staging/lustre/lustre/osc/osc_cache.c
+++ b/drivers/staging/lustre/lustre/osc/osc_cache.c
@@ -1557,15 +1557,22 @@ static bool osc_enter_cache_try(struct client_obd *cli,
 	}
 }
 
-static int ocw_granted(struct client_obd *cli, struct osc_cache_waiter *ocw)
-{
-	int rc;
-
-	spin_lock(&cli->cl_loi_list_lock);
-	rc = list_empty(&ocw->ocw_entry);
-	spin_unlock(&cli->cl_loi_list_lock);
-	return rc;
-}
+#define __wait_event_idle_exclusive_timeout_cmd(wq_head, condition,	\
+						timeout, cmd1, cmd2)	\
+	___wait_event(wq_head, ___wait_cond_timeout(condition),		\
+		      TASK_IDLE, 1, timeout,				\
+		      cmd1; __ret = schedule_timeout(__ret); cmd2)
+
+#define wait_event_idle_exclusive_timeout_cmd(wq_head, condition, timeout,\
+					      cmd1, cmd2)		\
+({									\
+	long __ret = timeout;						\
+	might_sleep();							\
+	if (!___wait_cond_timeout(condition))				\
+		__ret = __wait_event_idle_exclusive_timeout_cmd(	\
+			wq_head, condition, timeout, cmd1, cmd2);	\
+	__ret;								\
+})
 
 /**
  * The main entry to reserve dirty page accounting. Usually the grant reserved
@@ -1579,9 +1586,10 @@ static int osc_enter_cache(const struct lu_env *env, struct client_obd *cli,
 {
 	struct osc_object *osc = oap->oap_obj;
 	struct lov_oinfo *loi = osc->oo_oinfo;
-	struct osc_cache_waiter ocw;
 	unsigned long timeout = (AT_OFF ? obd_timeout : at_max) * HZ;
 	int rc = -EDQUOT;
+	int remain;
+	bool entered = false;
 
 	OSC_DUMP_GRANT(D_CACHE, cli, "need:%d\n", bytes);
 
@@ -1598,107 +1606,54 @@ static int osc_enter_cache(const struct lu_env *env, struct client_obd *cli,
 		goto out;
 	}
 
-	/* Hopefully normal case - cache space and write credits available */
-	if (osc_enter_cache_try(cli, oap, bytes, 0)) {
-		OSC_DUMP_GRANT(D_CACHE, cli, "granted from cache\n");
-		rc = 0;
-		goto out;
-	}
-
-	/* We can get here for two reasons: too many dirty pages in cache, or
+	/*
+	 * We can wait here for two reasons: too many dirty pages in cache, or
 	 * run out of grants. In both cases we should write dirty pages out.
 	 * Adding a cache waiter will trigger urgent write-out no matter what
 	 * RPC size will be.
-	 * The exiting condition is no avail grants and no dirty pages caching,
-	 * that really means there is no space on the OST.
+	 * The exiting condition (other then success) is no avail grants
+	 * and no dirty pages caching, that really means there is no space
+	 * on the OST.
 	 */
-	init_waitqueue_head(&ocw.ocw_waitq);
-	ocw.ocw_oap   = oap;
-	ocw.ocw_grant = bytes;
-	while (cli->cl_dirty_pages > 0 || cli->cl_w_in_flight > 0) {
-		list_add_tail(&ocw.ocw_entry, &cli->cl_cache_waiters);
-		ocw.ocw_rc = 0;
-		spin_unlock(&cli->cl_loi_list_lock);
+	remain = wait_event_idle_exclusive_timeout_cmd(
+		cli->cl_cache_waiters,
+		(entered = osc_enter_cache_try(
+			cli, oap, bytes, 0)) ||
+		(cli->cl_dirty_pages == 0 &&
+		 cli->cl_w_in_flight == 0),
+		timeout,
 
+		spin_unlock(&cli->cl_loi_list_lock);
 		osc_io_unplug_async(env, cli, NULL);
-
-		CDEBUG(D_CACHE, "%s: sleeping for cache space @ %p for %p\n",
-		       cli_name(cli), &ocw, oap);
-
-		rc = wait_event_idle_timeout(ocw.ocw_waitq,
-					     ocw_granted(cli, &ocw), timeout);
-
-		spin_lock(&cli->cl_loi_list_lock);
-
-		if (rc == 0) {
-			/* wait_event is interrupted by signal, or timed out */
-			list_del_init(&ocw.ocw_entry);
-			rc = -ETIMEDOUT;
-			break;
-		}
-		LASSERT(list_empty(&ocw.ocw_entry));
-		rc = ocw.ocw_rc;
-
-		if (rc != -EDQUOT)
-			break;
-		if (osc_enter_cache_try(cli, oap, bytes, 0)) {
-			rc = 0;
-			break;
-		}
-	}
-
-	switch (rc) {
-	case 0:
-		OSC_DUMP_GRANT(D_CACHE, cli, "finally got grant space\n");
-		break;
-	case -ETIMEDOUT:
+		CDEBUG(D_CACHE,
+		       "%s: sleeping for cache space for %p\n",
+		       cli_name(cli), oap);
+		,
+		spin_lock(&cli->cl_loi_list_lock));
+
+	if (entered) {
+		if (remain == timeout)
+			OSC_DUMP_GRANT(D_CACHE, cli, "granted from cache\n");
+		else
+			OSC_DUMP_GRANT(D_CACHE, cli,
+				       "finally got grant space\n");
+		wake_up(&cli->cl_cache_waiters);
+		rc = 0;
+	} else if (remain == 0) {
 		OSC_DUMP_GRANT(D_CACHE, cli,
 			       "timeout, fall back to sync i/o\n");
 		osc_extent_tree_dump(D_CACHE, osc);
 		/* fall back to synchronous I/O */
-		rc = -EDQUOT;
-		break;
-	case -EINTR:
-		/* Ensures restartability - LU-3581 */
-		OSC_DUMP_GRANT(D_CACHE, cli, "interrupted\n");
-		rc = -ERESTARTSYS;
-		break;
-	case -EDQUOT:
+	} else {
 		OSC_DUMP_GRANT(D_CACHE, cli,
 			       "no grant space, fall back to sync i/o\n");
-		break;
-	default:
-		CDEBUG(D_CACHE, "%s: event for cache space @ %p never arrived due to %d, fall back to sync i/o\n",
-		       cli_name(cli), &ocw, rc);
-		break;
+		wake_up_all(&cli->cl_cache_waiters);
 	}
 out:
 	spin_unlock(&cli->cl_loi_list_lock);
 	return rc;
 }
 
-/* caller must hold loi_list_lock */
-void osc_wake_cache_waiters(struct client_obd *cli)
-{
-	struct osc_cache_waiter *ocw;
-
-	while ((ocw = list_first_entry_or_null(&cli->cl_cache_waiters,
-					       struct osc_cache_waiter,
-					       ocw_entry))) {
-		list_del_init(&ocw->ocw_entry);
-
-		if (osc_enter_cache_try(cli, ocw->ocw_oap, ocw->ocw_grant, 0))
-			ocw->ocw_rc = 0;
-		else
-			ocw->ocw_rc = -EDQUOT;
-
-		CDEBUG(D_CACHE, "wake up %p for oap %p, avail grant %ld, %d\n",
-		       ocw, ocw->ocw_oap, cli->cl_avail_grant, ocw->ocw_rc);
-
-		wake_up(&ocw->ocw_waitq);
-	}
-}
-
 static int osc_max_rpc_in_flight(struct client_obd *cli, struct osc_object *osc)
 {
 	int hprpc = !!list_empty(&osc->oo_hp_exts);
@@ -1742,7 +1697,7 @@ static int osc_makes_rpc(struct client_obd *cli, struct osc_object *osc,
 		 * waiting for space.  as they're waiting, they're not going to
 		 * create more pages to coalesce with what's waiting..
 		 */
-		if (!list_empty(&cli->cl_cache_waiters)) {
+		if (waitqueue_active(&cli->cl_cache_waiters)) {
 			CDEBUG(D_CACHE, "cache waiters forcing RPC\n");
 			return 1;
 		}
@@ -2219,7 +2174,7 @@ static struct osc_object *osc_next_obj(struct client_obd *cli)
 	 * have filled up the cache and not been fired into rpcs because
 	 * they don't pass the nr_pending/object threshold
 	 */
-	if (!list_empty(&cli->cl_cache_waiters) &&
+	if (waitqueue_active(&cli->cl_cache_waiters) &&
 	    !list_empty(&cli->cl_loi_write_list))
 		return list_to_obj(&cli->cl_loi_write_list, write_item);
 
diff --git a/drivers/staging/lustre/lustre/osc/osc_internal.h b/drivers/staging/lustre/lustre/osc/osc_internal.h
index 0de8a3ee826d..0354272fe192 100644
--- a/drivers/staging/lustre/lustre/osc/osc_internal.h
+++ b/drivers/staging/lustre/lustre/osc/osc_internal.h
@@ -87,15 +87,11 @@ static inline struct osc_async_page *brw_page2oap(struct brw_page *pga)
 	return container_of(pga, struct osc_async_page, oap_brw_page);
 }
 
-struct osc_cache_waiter {
-	struct list_head		ocw_entry;
-	wait_queue_head_t		ocw_waitq;
-	struct osc_async_page		*ocw_oap;
-	int				ocw_grant;
-	int				ocw_rc;
-};
+static inline void osc_wake_cache_waiters(struct client_obd *cli)
+{
+	wake_up(&cli->cl_cache_waiters);
+}
 
-void osc_wake_cache_waiters(struct client_obd *cli);
 int osc_shrink_grant_to_target(struct client_obd *cli, __u64 target_bytes);
 void osc_update_next_shrink(struct client_obd *cli);
 
diff --git a/drivers/staging/lustre/lustre/osc/osc_page.c b/drivers/staging/lustre/lustre/osc/osc_page.c
index ada1eda24614..28b12729d7e9 100644
--- a/drivers/staging/lustre/lustre/osc/osc_page.c
+++ b/drivers/staging/lustre/lustre/osc/osc_page.c
@@ -155,7 +155,7 @@ static int osc_page_print(const struct lu_env *env,
 			  cli->cl_r_in_flight, cli->cl_w_in_flight,
 			  cli->cl_max_rpcs_in_flight,
 			  cli->cl_avail_grant,
-			  osc_list(&cli->cl_cache_waiters),
+			  waitqueue_active(&cli->cl_cache_waiters) ? "+" : "-",
 			  osc_list(&cli->cl_loi_ready_list),
 			  osc_list(&cli->cl_loi_hp_ready_list),
 			  osc_list(&cli->cl_loi_write_list),

[lustre-devel] [PATCH 16/29] lustre: osc_cache: use osc_makes_hprpc() more consistently.

[NeilBrown]

We have a function "osc_makes_hprpc()", and a few places
that could use it, but instead open-code the content.

Move the function earlier, and use it more broadly.

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

diff --git a/drivers/staging/lustre/lustre/osc/osc_cache.c b/drivers/staging/lustre/lustre/osc/osc_cache.c
index 9b78562d3366..d2d5867156ad 100644
--- a/drivers/staging/lustre/lustre/osc/osc_cache.c
+++ b/drivers/staging/lustre/lustre/osc/osc_cache.c
@@ -1654,9 +1654,14 @@ static int osc_enter_cache(const struct lu_env *env, struct client_obd *cli,
 	return rc;
 }
 
+static int osc_makes_hprpc(struct osc_object *obj)
+{
+	return !list_empty(&obj->oo_hp_exts);
+}
+
 static int osc_max_rpc_in_flight(struct client_obd *cli, struct osc_object *osc)
 {
-	int hprpc = !!list_empty(&osc->oo_hp_exts);
+	int hprpc = !osc_makes_hprpc(osc);
 
 	return rpcs_in_flight(cli) >= cli->cl_max_rpcs_in_flight + hprpc;
 }
@@ -1685,7 +1690,7 @@ static bool osc_makes_rpc(struct client_obd *cli, struct osc_object *osc,
 			CDEBUG(D_CACHE, "invalid import forcing RPC\n");
 			return true;
 		}
-		if (!list_empty(&osc->oo_hp_exts)) {
+		if (osc_makes_hprpc(osc)) {
 			CDEBUG(D_CACHE, "high prio request forcing RPC\n");
 			return true;
 		}
@@ -1736,11 +1741,6 @@ static void osc_update_pending(struct osc_object *obj, int cmd, int delta)
 	OSC_IO_DEBUG(obj, "update pending cmd %d delta %d.\n", cmd, delta);
 }
 
-static int osc_makes_hprpc(struct osc_object *obj)
-{
-	return !list_empty(&obj->oo_hp_exts);
-}
-
 static void on_list(struct list_head *item, struct list_head *list, int should_be_on)
 {
 	if (list_empty(item) && should_be_on)

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

[NeilBrown]

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(-)

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;
 		}

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

[Andreas Dilger]

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

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

[NeilBrown]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <http://lists.lustre.org/pipermail/lustre-devel-lustre.org/attachments/20190110/a2c410d6/attachment.sig>

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

[NeilBrown]

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",
-- 
2.14.0.rc0.dirty

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <http://lists.lustre.org/pipermail/lustre-devel-lustre.org/attachments/20190111/29144464/attachment.sig>

[lustre-devel] [PATCH 28/29] lustre: osc_request: assorted white-space and check-patch fixes.

[NeilBrown]

Just misc formatting fixes.  Some minot code change
where an 'else' after 'return' was discarded.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 drivers/staging/lustre/lustre/osc/osc_request.c |  163 +++++++++++++----------
 1 file changed, 89 insertions(+), 74 deletions(-)

diff --git a/drivers/staging/lustre/lustre/osc/osc_request.c b/drivers/staging/lustre/lustre/osc/osc_request.c
index ccc491efa982..c2239c99a7b2 100644
--- a/drivers/staging/lustre/lustre/osc/osc_request.c
+++ b/drivers/staging/lustre/lustre/osc/osc_request.c
@@ -63,37 +63,37 @@ static unsigned int osc_reqpool_mem_max = 5;
 module_param(osc_reqpool_mem_max, uint, 0444);
 
 struct osc_brw_async_args {
-	struct obdo       *aa_oa;
-	int		aa_requested_nob;
-	int		aa_nio_count;
-	u32		aa_page_count;
-	int		aa_resends;
-	struct brw_page  **aa_ppga;
-	struct client_obd *aa_cli;
-	struct list_head	 aa_oaps;
-	struct list_head	 aa_exts;
+	struct obdo		*aa_oa;
+	int			aa_requested_nob;
+	int			aa_nio_count;
+	u32			aa_page_count;
+	int			aa_resends;
+	struct brw_page		**aa_ppga;
+	struct client_obd	*aa_cli;
+	struct list_head	aa_oaps;
+	struct list_head	aa_exts;
 };
 
 struct osc_async_args {
-	struct obd_info   *aa_oi;
+	struct obd_info		*aa_oi;
 };
 
 struct osc_setattr_args {
-	struct obdo	 *sa_oa;
-	obd_enqueue_update_f sa_upcall;
-	void		*sa_cookie;
+	struct obdo		*sa_oa;
+	obd_enqueue_update_f	sa_upcall;
+	void			*sa_cookie;
 };
 
 struct osc_fsync_args {
 	struct osc_object	*fa_obj;
 	struct obdo		*fa_oa;
-	obd_enqueue_update_f fa_upcall;
-	void		*fa_cookie;
+	obd_enqueue_update_f	fa_upcall;
+	void			*fa_cookie;
 };
 
 struct osc_ladvise_args {
 	struct obdo		*la_oa;
-	obd_enqueue_update_f	 la_upcall;
+	obd_enqueue_update_f	la_upcall;
 	void			*la_cookie;
 };
 
@@ -101,12 +101,12 @@ struct osc_enqueue_args {
 	struct obd_export	*oa_exp;
 	enum ldlm_type		oa_type;
 	enum ldlm_mode		oa_mode;
-	__u64		    *oa_flags;
+	__u64			*oa_flags;
 	osc_enqueue_upcall_f	oa_upcall;
-	void		     *oa_cookie;
-	struct ost_lvb	   *oa_lvb;
+	void			*oa_cookie;
+	struct ost_lvb		*oa_lvb;
 	struct lustre_handle	oa_lockh;
-	unsigned int	      oa_agl:1;
+	unsigned int		oa_agl:1;
 };
 
 static void osc_release_ppga(struct brw_page **ppga, u32 count);
@@ -331,8 +331,7 @@ int osc_ladvise_base(struct obd_export *exp, struct obdo *oa,
 
 	body = req_capsule_client_get(&req->rq_pill, &RMF_OST_BODY);
 	LASSERT(body);
-	lustre_set_wire_obdo(&req->rq_import->imp_connect_data, &body->oa,
-			     oa);
+	lustre_set_wire_obdo(&req->rq_import->imp_connect_data, &body->oa, oa);
 
 	req_ladvise_hdr = req_capsule_client_get(&req->rq_pill,
 						 &RMF_OST_LADVISE_HDR);
@@ -652,8 +651,8 @@ static int osc_destroy(const struct lu_env *env, struct obd_export *exp,
 		 * Wait until the number of on-going destroy RPCs drops
 		 * under max_rpc_in_flight
 		 */
-		rc = l_wait_event_abortable_exclusive(cli->cl_destroy_waitq,
-						      osc_can_send_destroy(cli));
+		rc = l_wait_event_abortable_exclusive(
+			cli->cl_destroy_waitq, osc_can_send_destroy(cli));
 		if (rc) {
 			ptlrpc_req_finished(req);
 			return rc;
@@ -716,7 +715,8 @@ static void osc_announce_cached(struct client_obd *cli, struct obdo *oa,
 			 * take extent tax into account when asking for more
 			 * grant space
 			 */
-			nrextents = DIV_ROUND_UP(nrpages, cli->cl_max_extent_pages);
+			nrextents = DIV_ROUND_UP(nrpages,
+						 cli->cl_max_extent_pages);
 			oa->o_undirty += nrextents * cli->cl_grant_extent_tax;
 		}
 	}
@@ -930,7 +930,8 @@ static void osc_init_grant(struct client_obd *cli, struct obd_connect_data *ocd)
 		if (OCD_HAS_FLAG(ocd, GRANT_PARAM))
 			cli->cl_avail_grant -= cli->cl_dirty_grant;
 		else
-			cli->cl_avail_grant -= cli->cl_dirty_pages << PAGE_SHIFT;
+			cli->cl_avail_grant -=
+				cli->cl_dirty_pages << PAGE_SHIFT;
 	}
 
 	if (OCD_HAS_FLAG(ocd, GRANT_PARAM)) {
@@ -1181,11 +1182,12 @@ static int osc_brw_prep_request(int cmd, struct client_obd *cli,
 	 */
 	req->rq_no_retry_einprogress = 1;
 
-	desc = ptlrpc_prep_bulk_imp(req, page_count,
+	desc = ptlrpc_prep_bulk_imp(
+		req, page_count,
 		cli->cl_import->imp_connect_data.ocd_brw_size >> LNET_MTU_BITS,
 		(opc == OST_WRITE ? PTLRPC_BULK_GET_SOURCE :
 		 PTLRPC_BULK_PUT_SINK) | PTLRPC_BULK_BUF_KIOV, OST_BULK_PORTAL,
-		 &ptlrpc_bulk_kiov_pin_ops);
+		&ptlrpc_bulk_kiov_pin_ops);
 
 	if (!desc) {
 		rc = -ENOMEM;
@@ -1252,7 +1254,8 @@ static int osc_brw_prep_request(int cmd, struct client_obd *cli,
 		"want %p - real %p\n", req_capsule_client_get(&req->rq_pill,
 		&RMF_NIOBUF_REMOTE), (void *)(niobuf - niocount));
 
-	osc_announce_cached(cli, &body->oa, opc == OST_WRITE ? requested_nob:0);
+	osc_announce_cached(cli, &body->oa,
+			    opc == OST_WRITE ? requested_nob : 0);
 	if (resend) {
 		if ((body->oa.o_valid & OBD_MD_FLFLAGS) == 0) {
 			body->oa.o_valid |= OBD_MD_FLFLAGS;
@@ -1436,18 +1439,21 @@ static int check_write_checksum(struct obdo *oa,
 		msg = "changed in transit AND doesn't match the original - likely false positive due to mmap IO (bug 11742)"
 			;
 
-	LCONSOLE_ERROR_MSG(0x132,
-			   "%s: BAD WRITE CHECKSUM: %s: from %s inode " DFID " object " DOSTID " extent [%llu-%llu], original client csum %x (type %x), server csum %x (type %x), client csum now %x\n",
-			   aa->aa_cli->cl_import->imp_obd->obd_name,
-			   msg, libcfs_nid2str(peer->nid),
-			   oa->o_valid & OBD_MD_FLFID ? oa->o_parent_seq : (u64)0,
-			   oa->o_valid & OBD_MD_FLFID ? oa->o_parent_oid : 0,
-			   oa->o_valid & OBD_MD_FLFID ? oa->o_parent_ver : 0,
-			   POSTID(&oa->o_oi), aa->aa_ppga[0]->off,
-			   aa->aa_ppga[aa->aa_page_count - 1]->off +
-				aa->aa_ppga[aa->aa_page_count - 1]->count - 1,
-			   client_cksum, cksum_type_unpack(aa->aa_oa->o_flags),
-			   server_cksum, cksum_type, new_cksum);
+	LCONSOLE_ERROR_MSG(
+		0x132,
+		"%s: BAD WRITE CHECKSUM: %s: from %s inode " DFID
+		" object " DOSTID
+		" extent [%llu-%llu], original client csum %x (type %x), server csum %x (type %x), client csum now %x\n",
+		aa->aa_cli->cl_import->imp_obd->obd_name,
+		msg, libcfs_nid2str(peer->nid),
+		oa->o_valid & OBD_MD_FLFID ? oa->o_parent_seq : (u64)0,
+		oa->o_valid & OBD_MD_FLFID ? oa->o_parent_oid : 0,
+		oa->o_valid & OBD_MD_FLFID ? oa->o_parent_ver : 0,
+		POSTID(&oa->o_oi), aa->aa_ppga[0]->off,
+		aa->aa_ppga[aa->aa_page_count - 1]->off +
+		aa->aa_ppga[aa->aa_page_count - 1]->count - 1,
+		client_cksum, cksum_type_unpack(aa->aa_oa->o_flags),
+		server_cksum, cksum_type, new_cksum);
 
 	return 1;
 }
@@ -1480,7 +1486,8 @@ static int osc_brw_fini_request(struct ptlrpc_request *req, int rc)
 		unsigned int qid[MAXQUOTAS] = { body->oa.o_uid, body->oa.o_gid,
 						body->oa.o_projid };
 
-		CDEBUG(D_QUOTA, "setdq for [%u %u %u] with valid %#llx, flags %x\n",
+		CDEBUG(D_QUOTA,
+		       "setdq for [%u %u %u] with valid %#llx, flags %x\n",
 		       body->oa.o_uid, body->oa.o_gid, body->oa.o_projid,
 		       body->oa.o_valid, body->oa.o_flags);
 		osc_quota_setdq(cli, qid, body->oa.o_valid, body->oa.o_flags);
@@ -1569,23 +1576,26 @@ static int osc_brw_fini_request(struct ptlrpc_request *req, int rc)
 						    aa->aa_ppga, server_cksum,
 						    client_cksum);
 
-			LCONSOLE_ERROR_MSG(0x133,
-					   "%s: BAD READ CHECKSUM: from %s%s%s inode " DFID " object " DOSTID " extent [%llu-%llu], client %x, server %x, cksum_type %x\n",
-					   req->rq_import->imp_obd->obd_name,
-					   libcfs_nid2str(peer->nid),
-					   via, router,
-					   clbody->oa.o_valid & OBD_MD_FLFID ?
-						clbody->oa.o_parent_seq : (u64)0,
-					   clbody->oa.o_valid & OBD_MD_FLFID ?
-						clbody->oa.o_parent_oid : 0,
-					   clbody->oa.o_valid & OBD_MD_FLFID ?
-						clbody->oa.o_parent_ver : 0,
-					   POSTID(&body->oa.o_oi),
-					   aa->aa_ppga[0]->off,
-					   aa->aa_ppga[page_count - 1]->off +
-					   aa->aa_ppga[page_count - 1]->count - 1,
-					   client_cksum, server_cksum,
-					   cksum_type);
+			LCONSOLE_ERROR_MSG(
+				0x133,
+				"%s: BAD READ CHECKSUM: from %s%s%s inode " DFID
+				" object " DOSTID
+				" extent [%llu-%llu], client %x, server %x, cksum_type %x\n",
+				req->rq_import->imp_obd->obd_name,
+				libcfs_nid2str(peer->nid),
+				via, router,
+				clbody->oa.o_valid & OBD_MD_FLFID ?
+				clbody->oa.o_parent_seq : (u64)0,
+				clbody->oa.o_valid & OBD_MD_FLFID ?
+				clbody->oa.o_parent_oid : 0,
+				clbody->oa.o_valid & OBD_MD_FLFID ?
+				clbody->oa.o_parent_ver : 0,
+				POSTID(&body->oa.o_oi),
+				aa->aa_ppga[0]->off,
+				aa->aa_ppga[page_count - 1]->off +
+				aa->aa_ppga[page_count - 1]->count - 1,
+				client_cksum, server_cksum,
+				cksum_type);
 			cksum_counter = 0;
 			aa->aa_oa->o_cksum = client_cksum;
 			rc = -EAGAIN;
@@ -1622,8 +1632,8 @@ static int osc_brw_redo_request(struct ptlrpc_request *request,
 	DEBUG_REQ(rc == -EINPROGRESS ? D_RPCTRACE : D_ERROR, request,
 		  "redo for recoverable error %d", rc);
 
-	rc = osc_brw_prep_request(lustre_msg_get_opc(request->rq_reqmsg) ==
-					OST_WRITE ? OBD_BRW_WRITE : OBD_BRW_READ,
+	rc = osc_brw_prep_request((lustre_msg_get_opc(request->rq_reqmsg) ==
+				   OST_WRITE) ? OBD_BRW_WRITE : OBD_BRW_READ,
 				  aa->aa_cli, aa->aa_oa,
 				  aa->aa_page_count, aa->aa_ppga,
 				  &new_req, 0, 1);
@@ -1648,7 +1658,8 @@ static int osc_brw_redo_request(struct ptlrpc_request *request,
 	 * what ptlrpc does (see after_reply())
 	 */
 	if (aa->aa_resends > new_req->rq_timeout)
-		new_req->rq_sent = ktime_get_real_seconds() + new_req->rq_timeout;
+		new_req->rq_sent = (ktime_get_real_seconds() +
+				    new_req->rq_timeout);
 	else
 		new_req->rq_sent = ktime_get_real_seconds() + aa->aa_resends;
 	new_req->rq_generation_set = 1;
@@ -1702,7 +1713,8 @@ static void sort_brw_pages(struct brw_page **array, int num)
 		for (i = stride ; i < num ; i++) {
 			tmp = array[i];
 			j = i;
-			while (j >= stride && array[j - stride]->off > tmp->off) {
+			while (j >= stride &&
+			       array[j - stride]->off > tmp->off) {
 				array[j] = array[j - stride];
 				j -= stride;
 			}
@@ -1733,7 +1745,9 @@ static int brw_interpret(const struct lu_env *env,
 	if (osc_recoverable_error(rc)) {
 		if (req->rq_import_generation !=
 		    req->rq_import->imp_generation) {
-			CDEBUG(D_HA, "%s: resend cross eviction for object: " DOSTID ", rc = %d.\n",
+			CDEBUG(D_HA,
+			       "%s: resend cross eviction for object: " DOSTID
+			       ", rc = %d.\n",
 			       req->rq_import->imp_obd->obd_name,
 			       POSTID(&aa->aa_oa->o_oi), rc);
 		} else if (rc == -EINPROGRESS ||
@@ -2093,7 +2107,6 @@ static int osc_enqueue_interpret(const struct lu_env *env,
 	__u32 lvb_len = sizeof(*lvb);
 	__u64 flags = 0;
 
-
 	/* ldlm_cli_enqueue is holding a reference on the lock, so it must
 	 * be valid.
 	 */
@@ -2212,7 +2225,8 @@ int osc_enqueue_base(struct obd_export *exp, struct ldlm_res_id *res_id,
 			ldlm_lock_decref(&lockh, mode);
 			LDLM_LOCK_PUT(matched);
 			return -ECANCELED;
-		} else if (osc_set_lock_data(matched, einfo->ei_cbdata)) {
+		}
+		if (osc_set_lock_data(matched, einfo->ei_cbdata)) {
 			*flags |= LDLM_FL_LVB_READY;
 			/* We already have a lock, and it's referenced. */
 			(*upcall)(cookie, &lockh, ELDLM_LOCK_MATCHED);
@@ -2220,10 +2234,9 @@ int osc_enqueue_base(struct obd_export *exp, struct ldlm_res_id *res_id,
 			ldlm_lock_decref(&lockh, mode);
 			LDLM_LOCK_PUT(matched);
 			return ELDLM_OK;
-		} else {
-			ldlm_lock_decref(&lockh, mode);
-			LDLM_LOCK_PUT(matched);
 		}
+		ldlm_lock_decref(&lockh, mode);
+		LDLM_LOCK_PUT(matched);
 	}
 
 no_match:
@@ -2269,9 +2282,9 @@ int osc_enqueue_base(struct obd_export *exp, struct ldlm_res_id *res_id,
 				aa->oa_lvb = lvb;
 			} else {
 				/* AGL is essentially to enqueue an DLM lock
-				* in advance, so we don't care about the
-				* result of AGL enqueue.
-				*/
+				 * in advance, so we don't care about the
+				 * result of AGL enqueue.
+				 */
 				aa->oa_lvb = NULL;
 				aa->oa_flags = NULL;
 			}
@@ -2663,7 +2676,8 @@ static int osc_reconnect(const struct lu_env *env,
 		cli->cl_lost_grant = 0;
 		spin_unlock(&cli->cl_loi_list_lock);
 
-		CDEBUG(D_RPCTRACE, "ocd_connect_flags: %#llx ocd_version: %d ocd_grant: %d, lost: %ld.\n",
+		CDEBUG(D_RPCTRACE,
+		       "ocd_connect_flags: %#llx ocd_version: %d ocd_grant: %d, lost: %ld.\n",
 		       data->ocd_connect_flags,
 		       data->ocd_version, data->ocd_grant, lost_grant);
 	}
@@ -2788,7 +2802,8 @@ static int osc_import_event(struct obd_device *obd,
 
 		/* See bug 7198 */
 		if (ocd->ocd_connect_flags & OBD_CONNECT_REQPORTAL)
-			imp->imp_client->cli_request_portal = OST_REQUEST_PORTAL;
+			imp->imp_client->cli_request_portal =
+				OST_REQUEST_PORTAL;
 
 		rc = obd_notify_observer(obd, obd, OBD_NOTIFY_OCD);
 		break;

[lustre-devel] [PATCH 28/29] lustre: osc_request: assorted white-space and check-patch fixes.

[Andreas Dilger]

On Jan 8, 2019, at 23:24, NeilBrown <neilb@suse.com> wrote:
> 
> Just misc formatting fixes.  Some minot code change

(typo) "minor"

> where an 'else' after 'return' was discarded.
> 
> Signed-off-by: NeilBrown <neilb@suse.com>
> ---
> drivers/staging/lustre/lustre/osc/osc_request.c |  163 +++++++++++++----------
> 1 file changed, 89 insertions(+), 74 deletions(-)
> 
> diff --git a/drivers/staging/lustre/lustre/osc/osc_request.c b/drivers/staging/lustre/lustre/osc/osc_request.c
> index ccc491efa982..c2239c99a7b2 100644
> --- a/drivers/staging/lustre/lustre/osc/osc_request.c
> +++ b/drivers/staging/lustre/lustre/osc/osc_request.c
> 
> @@ -652,8 +651,8 @@ static int osc_destroy(const struct lu_env *env, struct obd_export *exp,
> 		 * Wait until the number of on-going destroy RPCs drops
> 		 * under max_rpc_in_flight
> 		 */
> -		rc = l_wait_event_abortable_exclusive(cli->cl_destroy_waitq,
> -						      osc_can_send_destroy(cli));
> +		rc = l_wait_event_abortable_exclusive(
> +			cli->cl_destroy_waitq, osc_can_send_destroy(cli));
> 		if (rc) {
> 			ptlrpc_req_finished(req);
> 			return rc;

I don't really see this change as an improvement?  I'd instead just de-indent
the osc_can_send_destroy() by a space or two so it fits within 80 colunmns.
That is IMHO closer to the normal coding style.

Cheers, Andreas
---
Andreas Dilger
Principal Lustre Architect
Whamcloud

[lustre-devel] [PATCH 28/29] lustre: osc_request: assorted white-space and check-patch fixes.

[NeilBrown]

On Thu, Jan 10 2019, Andreas Dilger wrote:

> On Jan 8, 2019, at 23:24, NeilBrown <neilb@suse.com> wrote:
>> 
>> Just misc formatting fixes.  Some minot code change
>
> (typo) "minor"
>
>> where an 'else' after 'return' was discarded.
>> 
>> Signed-off-by: NeilBrown <neilb@suse.com>
>> ---
>> drivers/staging/lustre/lustre/osc/osc_request.c |  163 +++++++++++++----------
>> 1 file changed, 89 insertions(+), 74 deletions(-)
>> 
>> diff --git a/drivers/staging/lustre/lustre/osc/osc_request.c b/drivers/staging/lustre/lustre/osc/osc_request.c
>> index ccc491efa982..c2239c99a7b2 100644
>> --- a/drivers/staging/lustre/lustre/osc/osc_request.c
>> +++ b/drivers/staging/lustre/lustre/osc/osc_request.c
>> 
>> @@ -652,8 +651,8 @@ static int osc_destroy(const struct lu_env *env, struct obd_export *exp,
>> 		 * Wait until the number of on-going destroy RPCs drops
>> 		 * under max_rpc_in_flight
>> 		 */
>> -		rc = l_wait_event_abortable_exclusive(cli->cl_destroy_waitq,
>> -						      osc_can_send_destroy(cli));
>> +		rc = l_wait_event_abortable_exclusive(
>> +			cli->cl_destroy_waitq, osc_can_send_destroy(cli));
>> 		if (rc) {
>> 			ptlrpc_req_finished(req);
>> 			return rc;
>
> I don't really see this change as an improvement?  I'd instead just de-indent
> the osc_can_send_destroy() by a space or two so it fits within 80 colunmns.
> That is IMHO closer to the normal coding style.

That would result in:

CHECK: Alignment should match open parenthesis
#655: FILE: drivers/staging/lustre/lustre/osc/osc_request.c:655:
+		rc = l_wait_event_abortable_exclusive(cli->cl_destroy_waitq,
+						    osc_can_send_destroy(cli));

though now we have:

CHECK: Lines should not end with a '('
#654: FILE: drivers/staging/lustre/lustre/osc/osc_request.c:654:
+		rc = l_wait_event_abortable_exclusive(

I might just leave it as it is, with an 81-column line.

Thanks,
NeilBrown


>
> Cheers, Andreas
> ---
> Andreas Dilger
> Principal Lustre Architect
> Whamcloud
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <http://lists.lustre.org/pipermail/lustre-devel-lustre.org/attachments/20190110/937db209/attachment.sig>

[lustre-devel] [PATCH 26/29] lustre: osc_cache: simplify osc_page_gang_lookup()

[NeilBrown]

osc_page_gang_lookup() has 4 values that it can receive from a
callback, and that it can return to the caller:
	CLP_GANG_OKAY,
	CLP_GANG_RESCHED,
	CLP_GANG_AGAIN,
	CLP_GANG_ABORT

"AGAIN" is never used.
"RESCHED" is not needed as a cond_resched() can safely be called at
the point this is returned, rather than returning it.
That leaves "OKAY" and "ABORT" which can simply by "true" and "false"
boolean values.

Internalizing the RESCHED case means the callers don't need to loop
themselves.  This simplify calling patterns.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 drivers/staging/lustre/lustre/include/cl_object.h  |    7 ----
 drivers/staging/lustre/lustre/osc/osc_cache.c      |   40 ++++++++------------
 .../staging/lustre/lustre/osc/osc_cl_internal.h    |   10 +++--
 drivers/staging/lustre/lustre/osc/osc_io.c         |    4 +-
 drivers/staging/lustre/lustre/osc/osc_lock.c       |   27 ++++++--------
 5 files changed, 33 insertions(+), 55 deletions(-)

diff --git a/drivers/staging/lustre/lustre/include/cl_object.h b/drivers/staging/lustre/lustre/include/cl_object.h
index de5d68879740..57d8fe676995 100644
--- a/drivers/staging/lustre/lustre/include/cl_object.h
+++ b/drivers/staging/lustre/lustre/include/cl_object.h
@@ -2076,14 +2076,7 @@ static inline int cl_object_refc(struct cl_object *clob)
 /** \defgroup cl_page cl_page
  * @{
  */
-enum {
-	CLP_GANG_OKAY = 0,
-	CLP_GANG_RESCHED,
-	CLP_GANG_AGAIN,
-	CLP_GANG_ABORT
-};
 
-/* callback of cl_page_gang_lookup() */
 struct cl_page *cl_page_find(const struct lu_env *env, struct cl_object *obj,
 			     pgoff_t idx, struct page *vmpage,
 			     enum cl_page_type type);
diff --git a/drivers/staging/lustre/lustre/osc/osc_cache.c b/drivers/staging/lustre/lustre/osc/osc_cache.c
index 79bcaa212339..e01f3815978c 100644
--- a/drivers/staging/lustre/lustre/osc/osc_cache.c
+++ b/drivers/staging/lustre/lustre/osc/osc_cache.c
@@ -2994,18 +2994,14 @@ int osc_cache_writeback_range(const struct lu_env *env, struct osc_object *obj,
 /**
  * Returns a list of pages by a given [start, end] of \a obj.
  *
- * \param resched If not NULL, then we give up before hogging CPU for too
- * long and set *resched = 1, in that case caller should implement a retry
- * logic.
- *
  * Gang tree lookup (radix_tree_gang_lookup()) optimization is absolutely
  * crucial in the face of [offset, EOF] locks.
  *
  * Return at least one page in @queue unless there is no covered page.
  */
-int osc_page_gang_lookup(const struct lu_env *env, struct cl_io *io,
-			 struct osc_object *osc, pgoff_t start, pgoff_t end,
-			 osc_page_gang_cbt cb, void *cbdata)
+bool osc_page_gang_lookup(const struct lu_env *env, struct cl_io *io,
+			  struct osc_object *osc, pgoff_t start, pgoff_t end,
+			  osc_page_gang_cbt cb, void *cbdata)
 {
 	struct osc_page *ops;
 	void            **pvec;
@@ -3013,7 +3009,7 @@ int osc_page_gang_lookup(const struct lu_env *env, struct cl_io *io,
 	unsigned int    nr;
 	unsigned int    i;
 	unsigned int    j;
-	int             res = CLP_GANG_OKAY;
+	bool            res = true;
 	bool            tree_lock = true;
 
 	idx = start;
@@ -3059,7 +3055,7 @@ int osc_page_gang_lookup(const struct lu_env *env, struct cl_io *io,
 
 		for (i = 0; i < j; ++i) {
 			ops = pvec[i];
-			if (res == CLP_GANG_OKAY)
+			if (res)
 				res = (*cb)(env, io, ops, cbdata);
 
 			page = ops->ops_cl.cpl_page;
@@ -3069,10 +3065,10 @@ int osc_page_gang_lookup(const struct lu_env *env, struct cl_io *io,
 		if (nr < OTI_PVEC_SIZE || end_of_region)
 			break;
 
-		if (res == CLP_GANG_OKAY && need_resched())
-			res = CLP_GANG_RESCHED;
-		if (res != CLP_GANG_OKAY)
+		if (!res)
 			break;
+		if (need_resched())
+			cond_resched();
 
 		spin_lock(&osc->oo_tree_lock);
 		tree_lock = true;
@@ -3085,7 +3081,7 @@ int osc_page_gang_lookup(const struct lu_env *env, struct cl_io *io,
 /**
  * Check if page @page is covered by an extra lock or discard it.
  */
-static int check_and_discard_cb(const struct lu_env *env, struct cl_io *io,
+static bool check_and_discard_cb(const struct lu_env *env, struct cl_io *io,
 				struct osc_page *ops, void *cbdata)
 {
 	struct osc_thread_info *info = osc_env_info(env);
@@ -3121,10 +3117,10 @@ static int check_and_discard_cb(const struct lu_env *env, struct cl_io *io,
 	}
 
 	info->oti_next_index = index + 1;
-	return CLP_GANG_OKAY;
+	return true;
 }
 
-static int discard_cb(const struct lu_env *env, struct cl_io *io,
+static bool discard_cb(const struct lu_env *env, struct cl_io *io,
 		      struct osc_page *ops, void *cbdata)
 {
 	struct osc_thread_info *info = osc_env_info(env);
@@ -3145,7 +3141,7 @@ static int discard_cb(const struct lu_env *env, struct cl_io *io,
 		LASSERT(page->cp_state == CPS_FREEING);
 	}
 
-	return CLP_GANG_OKAY;
+	return true;
 }
 
 /**
@@ -3162,7 +3158,7 @@ int osc_lock_discard_pages(const struct lu_env *env, struct osc_object *osc,
 	struct osc_thread_info *info = osc_env_info(env);
 	struct cl_io *io = &info->oti_io;
 	osc_page_gang_cbt cb;
-	int res;
+	bool res;
 	int result;
 
 	io->ci_obj = cl_object_top(osc2cl(osc));
@@ -3174,15 +3170,9 @@ int osc_lock_discard_pages(const struct lu_env *env, struct osc_object *osc,
 	cb = discard ? discard_cb : check_and_discard_cb;
 	info->oti_fn_index = start;
 	info->oti_next_index = start;
-	do {
-		res = osc_page_gang_lookup(env, io, osc,
-					   info->oti_next_index, end, cb, osc);
-		if (info->oti_next_index > end)
-			break;
 
-		if (res == CLP_GANG_RESCHED)
-			cond_resched();
-	} while (res != CLP_GANG_OKAY);
+	res = osc_page_gang_lookup(env, io, osc,
+				   info->oti_next_index, end, cb, osc);
 out:
 	cl_io_fini(env, io);
 	return result;
diff --git a/drivers/staging/lustre/lustre/osc/osc_cl_internal.h b/drivers/staging/lustre/lustre/osc/osc_cl_internal.h
index 3af096e0dbdd..c0f58f41513f 100644
--- a/drivers/staging/lustre/lustre/osc/osc_cl_internal.h
+++ b/drivers/staging/lustre/lustre/osc/osc_cl_internal.h
@@ -660,11 +660,11 @@ void osc_extent_release(const struct lu_env *env, struct osc_extent *ext);
 int osc_lock_discard_pages(const struct lu_env *env, struct osc_object *osc,
 			   pgoff_t start, pgoff_t end, bool discard_pages);
 
-typedef int (*osc_page_gang_cbt)(const struct lu_env *, struct cl_io *,
-				 struct osc_page *, void *);
-int osc_page_gang_lookup(const struct lu_env *env, struct cl_io *io,
-			 struct osc_object *osc, pgoff_t start, pgoff_t end,
-			 osc_page_gang_cbt cb, void *cbdata);
+typedef bool (*osc_page_gang_cbt)(const struct lu_env *, struct cl_io *,
+				  struct osc_page *, void *);
+bool osc_page_gang_lookup(const struct lu_env *env, struct cl_io *io,
+			  struct osc_object *osc, pgoff_t start, pgoff_t end,
+			  osc_page_gang_cbt cb, void *cbdata);
 /* @} osc */
 
 #endif /* OSC_CL_INTERNAL_H */
diff --git a/drivers/staging/lustre/lustre/osc/osc_io.c b/drivers/staging/lustre/lustre/osc/osc_io.c
index dabdf6da8b38..b95e0358d7ff 100644
--- a/drivers/staging/lustre/lustre/osc/osc_io.c
+++ b/drivers/staging/lustre/lustre/osc/osc_io.c
@@ -449,7 +449,7 @@ static int osc_async_upcall(void *a, int rc)
 /**
  * Checks that there are no pages being written in the extent being truncated.
  */
-static int trunc_check_cb(const struct lu_env *env, struct cl_io *io,
+static bool trunc_check_cb(const struct lu_env *env, struct cl_io *io,
 			  struct osc_page *ops, void *cbdata)
 {
 	struct cl_page *page = ops->ops_cl.cpl_page;
@@ -466,7 +466,7 @@ static int trunc_check_cb(const struct lu_env *env, struct cl_io *io,
 		CDEBUG(D_CACHE, "page %p index %lu locked for %d.\n",
 		       ops, osc_index(ops), oap->oap_cmd & OBD_BRW_RWMASK);
 
-	return CLP_GANG_OKAY;
+	return true;
 }
 
 static void osc_trunc_check(const struct lu_env *env, struct cl_io *io,
diff --git a/drivers/staging/lustre/lustre/osc/osc_lock.c b/drivers/staging/lustre/lustre/osc/osc_lock.c
index 4cc813d192d9..1eab61d720e2 100644
--- a/drivers/staging/lustre/lustre/osc/osc_lock.c
+++ b/drivers/staging/lustre/lustre/osc/osc_lock.c
@@ -630,18 +630,18 @@ static int osc_ldlm_glimpse_ast(struct ldlm_lock *dlmlock, void *data)
 	return result;
 }
 
-static int weigh_cb(const struct lu_env *env, struct cl_io *io,
-		    struct osc_page *ops, void *cbdata)
+static bool weigh_cb(const struct lu_env *env, struct cl_io *io,
+		     struct osc_page *ops, void *cbdata)
 {
 	struct cl_page *page = ops->ops_cl.cpl_page;
 
 	if (cl_page_is_vmlocked(env, page) ||
 	    PageDirty(page->cp_vmpage) || PageWriteback(page->cp_vmpage)
 	   )
-		return CLP_GANG_ABORT;
+		return false;
 
 	*(pgoff_t *)cbdata = osc_index(ops) + 1;
-	return CLP_GANG_OKAY;
+	return true;
 }
 
 static unsigned long osc_lock_weight(const struct lu_env *env,
@@ -651,7 +651,7 @@ static unsigned long osc_lock_weight(const struct lu_env *env,
 	struct cl_io *io = &osc_env_info(env)->oti_io;
 	struct cl_object *obj = cl_object_top(&oscobj->oo_cl);
 	pgoff_t page_index;
-	int result;
+	bool result;
 
 	io->ci_obj = obj;
 	io->ci_ignore_layout = 1;
@@ -660,19 +660,14 @@ static unsigned long osc_lock_weight(const struct lu_env *env,
 		return result;
 
 	page_index = cl_index(obj, extent->start);
-	do {
-		result = osc_page_gang_lookup(env, io, oscobj,
-					      page_index,
-					      cl_index(obj, extent->end),
-					      weigh_cb, (void *)&page_index);
-		if (result == CLP_GANG_ABORT)
-			break;
-		if (result == CLP_GANG_RESCHED)
-			cond_resched();
-	} while (result != CLP_GANG_OKAY);
+
+	result = osc_page_gang_lookup(env, io, oscobj,
+				      page_index,
+				      cl_index(obj, extent->end),
+				      weigh_cb, (void *)&page_index);
 	cl_io_fini(env, io);
 
-	return result == CLP_GANG_ABORT ? 1 : 0;
+	return result ? 1 : 0;
 }
 
 /**

[lustre-devel] [PATCH 26/29] lustre: osc_cache: simplify osc_page_gang_lookup()

[Andreas Dilger]

On Jan 8, 2019, at 23:24, NeilBrown <neilb@suse.com> wrote:
> 
> osc_page_gang_lookup() has 4 values that it can receive from a
> callback, and that it can return to the caller:
> 	CLP_GANG_OKAY,
> 	CLP_GANG_RESCHED,
> 	CLP_GANG_AGAIN,
> 	CLP_GANG_ABORT
> 
> "AGAIN" is never used.
> "RESCHED" is not needed as a cond_resched() can safely be called at
> the point this is returned, rather than returning it.
> That leaves "OKAY" and "ABORT" which can simply by "true" and "false"
> boolean values.
> 
> Internalizing the RESCHED case means the callers don't need to loop
> themselves.  This simplify calling patterns.
> 
> Signed-off-by: NeilBrown <neilb@suse.com>
> ---
> drivers/staging/lustre/lustre/include/cl_object.h  |    7 ----
> drivers/staging/lustre/lustre/osc/osc_cache.c      |   40 ++++++++------------
> .../staging/lustre/lustre/osc/osc_cl_internal.h    |   10 +++--
> drivers/staging/lustre/lustre/osc/osc_io.c         |    4 +-
> drivers/staging/lustre/lustre/osc/osc_lock.c       |   27 ++++++--------
> 5 files changed, 33 insertions(+), 55 deletions(-)
> 
> diff --git a/drivers/staging/lustre/lustre/osc/osc_cache.c b/drivers/staging/lustre/lustre/osc/osc_cache.c
> index 79bcaa212339..e01f3815978c 100644
> --- a/drivers/staging/lustre/lustre/osc/osc_cache.c
> +++ b/drivers/staging/lustre/lustre/osc/osc_cache.c
> @@ -3069,10 +3065,10 @@ int osc_page_gang_lookup(const struct lu_env *env, struct cl_io *io,
> 		if (nr < OTI_PVEC_SIZE || end_of_region)
> 			break;
> 
> -		if (res == CLP_GANG_OKAY && need_resched())
> -			res = CLP_GANG_RESCHED;
> -		if (res != CLP_GANG_OKAY)
> +		if (!res)
> 			break;
> +		if (need_resched())
> +			cond_resched();
> 
> 		spin_lock(&osc->oo_tree_lock);
> 		tree_lock = true;

The one thing I notice here is that if the CLP_GANG_RESCHED is not
returned to the caller, it doesn't have the chance to finish the
work before it is rescheduled:

        do {
                res = osc_page_gang_lookup(env, io, osc,
                                   info->oti_next_index, end, cb, osc);
                if (info->oti_next_index > end)
                        break;

                if (res == CLP_GANG_RESCHED)
                        cond_resched();
        } while (res != CLP_GANG_OKAY);

That means if the thread did a lot of work in osc_page_gang_lookup()
but is otherwise finished, it will block at the internal cond_resched()
rather than detecting it is finishing and returning to the caller without
any reschedule at all.

However, looking into the osc_page_gang_lookup() code more closely, I
see "end_of_region" would already be set in this case (it is just at
the start of the context in the above patch hunk) so CLP_GANG_RESCHED
should never be set in that case.  So it looks OK.

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

> diff --git a/drivers/staging/lustre/lustre/osc/osc_lock.c b/drivers/staging/lustre/lustre/osc/osc_lock.c
> index 4cc813d192d9..1eab61d720e2 100644
> --- a/drivers/staging/lustre/lustre/osc/osc_lock.c
> +++ b/drivers/staging/lustre/lustre/osc/osc_lock.c
> @@ -630,18 +630,18 @@ static int osc_ldlm_glimpse_ast(struct ldlm_lock *dlmlock, void *data)
> 	return result;
> }
> 
> -static int weigh_cb(const struct lu_env *env, struct cl_io *io,
> -		    struct osc_page *ops, void *cbdata)
> +static bool weigh_cb(const struct lu_env *env, struct cl_io *io,
> +		     struct osc_page *ops, void *cbdata)
> {
> 	struct cl_page *page = ops->ops_cl.cpl_page;
> 
> 	if (cl_page_is_vmlocked(env, page) ||
> 	    PageDirty(page->cp_vmpage) || PageWriteback(page->cp_vmpage)
> 	   )

This is a bit oddly formatted.  I see in our tree it looks like:

        if (cl_page_is_vmlocked(env, page) || PageDirty(page->cp_vmpage) ||
            PageWriteback(page->cp_vmpage))

which is more normal.

> @@ -660,19 +660,14 @@ static unsigned long osc_lock_weight(const struct lu_env *env,
> 		return result;
> 
> 	page_index = cl_index(obj, extent->start);
> +
> +	result = osc_page_gang_lookup(env, io, oscobj,
> +				      page_index,
> +				      cl_index(obj, extent->end),
> +				      weigh_cb, (void *)&page_index);
> 	cl_io_fini(env, io);
> 
> -	return result == CLP_GANG_ABORT ? 1 : 0;
> +	return result ? 1 : 0;
> }

Per your commit comment above:

> That leaves "OKAY" and "ABORT" which can simply by "true" and "false"
> boolean values.

So if "ABORT" is now "false", this should be:

	return !result;

otherwise your return code logic is backward?

Cheers, Andreas
---
Andreas Dilger
Principal Lustre Architect
Whamcloud

[lustre-devel] [PATCH 26/29] lustre: osc_cache: simplify osc_page_gang_lookup()

[NeilBrown]

On Thu, Jan 10 2019, Andreas Dilger wrote:

> On Jan 8, 2019, at 23:24, NeilBrown <neilb@suse.com> wrote:
>> 
>> osc_page_gang_lookup() has 4 values that it can receive from a
>> callback, and that it can return to the caller:
>> 	CLP_GANG_OKAY,
>> 	CLP_GANG_RESCHED,
>> 	CLP_GANG_AGAIN,
>> 	CLP_GANG_ABORT
>> 
>> "AGAIN" is never used.
>> "RESCHED" is not needed as a cond_resched() can safely be called at
>> the point this is returned, rather than returning it.
>> That leaves "OKAY" and "ABORT" which can simply by "true" and "false"
>> boolean values.
>> 
>> Internalizing the RESCHED case means the callers don't need to loop
>> themselves.  This simplify calling patterns.
>> 
>> Signed-off-by: NeilBrown <neilb@suse.com>
>> ---
>> drivers/staging/lustre/lustre/include/cl_object.h  |    7 ----
>> drivers/staging/lustre/lustre/osc/osc_cache.c      |   40 ++++++++------------
>> .../staging/lustre/lustre/osc/osc_cl_internal.h    |   10 +++--
>> drivers/staging/lustre/lustre/osc/osc_io.c         |    4 +-
>> drivers/staging/lustre/lustre/osc/osc_lock.c       |   27 ++++++--------
>> 5 files changed, 33 insertions(+), 55 deletions(-)
>> 
>> diff --git a/drivers/staging/lustre/lustre/osc/osc_cache.c b/drivers/staging/lustre/lustre/osc/osc_cache.c
>> index 79bcaa212339..e01f3815978c 100644
>> --- a/drivers/staging/lustre/lustre/osc/osc_cache.c
>> +++ b/drivers/staging/lustre/lustre/osc/osc_cache.c
>> @@ -3069,10 +3065,10 @@ int osc_page_gang_lookup(const struct lu_env *env, struct cl_io *io,
>> 		if (nr < OTI_PVEC_SIZE || end_of_region)
>> 			break;
>> 
>> -		if (res == CLP_GANG_OKAY && need_resched())
>> -			res = CLP_GANG_RESCHED;
>> -		if (res != CLP_GANG_OKAY)
>> +		if (!res)
>> 			break;
>> +		if (need_resched())
>> +			cond_resched();
>> 
>> 		spin_lock(&osc->oo_tree_lock);
>> 		tree_lock = true;
>
> The one thing I notice here is that if the CLP_GANG_RESCHED is not
> returned to the caller, it doesn't have the chance to finish the
> work before it is rescheduled:
>
>         do {
>                 res = osc_page_gang_lookup(env, io, osc,
>                                    info->oti_next_index, end, cb, osc);
>                 if (info->oti_next_index > end)
>                         break;
>
>                 if (res == CLP_GANG_RESCHED)
>                         cond_resched();
>         } while (res != CLP_GANG_OKAY);
>
> That means if the thread did a lot of work in osc_page_gang_lookup()
> but is otherwise finished, it will block at the internal cond_resched()
> rather than detecting it is finishing and returning to the caller without
> any reschedule at all.
>
> However, looking into the osc_page_gang_lookup() code more closely, I
> see "end_of_region" would already be set in this case (it is just at
> the start of the context in the above patch hunk) so CLP_GANG_RESCHED
> should never be set in that case.  So it looks OK.

Oh good :-)
Thanks.
I love when a review includes what you saw as well as the "Reviewed-by".

>
> Reviewed-by: Andreas Dilger <adilger@whamcloud.com>
>
>> diff --git a/drivers/staging/lustre/lustre/osc/osc_lock.c b/drivers/staging/lustre/lustre/osc/osc_lock.c
>> index 4cc813d192d9..1eab61d720e2 100644
>> --- a/drivers/staging/lustre/lustre/osc/osc_lock.c
>> +++ b/drivers/staging/lustre/lustre/osc/osc_lock.c
>> @@ -630,18 +630,18 @@ static int osc_ldlm_glimpse_ast(struct ldlm_lock *dlmlock, void *data)
>> 	return result;
>> }
>> 
>> -static int weigh_cb(const struct lu_env *env, struct cl_io *io,
>> -		    struct osc_page *ops, void *cbdata)
>> +static bool weigh_cb(const struct lu_env *env, struct cl_io *io,
>> +		     struct osc_page *ops, void *cbdata)
>> {
>> 	struct cl_page *page = ops->ops_cl.cpl_page;
>> 
>> 	if (cl_page_is_vmlocked(env, page) ||
>> 	    PageDirty(page->cp_vmpage) || PageWriteback(page->cp_vmpage)
>> 	   )
>
> This is a bit oddly formatted.  I see in our tree it looks like:
>
>         if (cl_page_is_vmlocked(env, page) || PageDirty(page->cp_vmpage) ||
>             PageWriteback(page->cp_vmpage))
>
> which is more normal.

It has only been this way in OpenSFS since July this year.

Commit b44b1ff8c7fc ("LU-10961 ldlm: don't cancel DoM locks before replay")
made the change without any comment.  I guess we aren't up to that 2.11
yet :-)



>
>> @@ -660,19 +660,14 @@ static unsigned long osc_lock_weight(const struct lu_env *env,
>> 		return result;
>> 
>> 	page_index = cl_index(obj, extent->start);
>> +
>> +	result = osc_page_gang_lookup(env, io, oscobj,
>> +				      page_index,
>> +				      cl_index(obj, extent->end),
>> +				      weigh_cb, (void *)&page_index);
>> 	cl_io_fini(env, io);
>> 
>> -	return result == CLP_GANG_ABORT ? 1 : 0;
>> +	return result ? 1 : 0;
>> }
>
> Per your commit comment above:
>
>> That leaves "OKAY" and "ABORT" which can simply by "true" and "false"
>> boolean values.
>
> So if "ABORT" is now "false", this should be:
>
> 	return !result;
>
> otherwise your return code logic is backward?

Good catch, thanks.
That isn't the only problem there.  "result" - which I change to a bool
- also holds the return value for cl_io_init().  I'm not sur what sort
of value that is, but I don't think I should be changing the type.
So I changed 'result' back to an int and handled failure differently.
I also removed the 'res' variable from osc_Lock_discard_pages(), as it
is now unused.

New version below.

Thanks,
NeilBrown

From: NeilBrown <neilb@suse.com>
Subject: [PATCH] lustre: osc_cache: simplify osc_page_gang_lookup()

osc_page_gang_lookup() has 4 values that it can receive from a
callback, and that it can return to the caller:
	CLP_GANG_OKAY,
	CLP_GANG_RESCHED,
	CLP_GANG_AGAIN,
	CLP_GANG_ABORT

"AGAIN" is never used.
"RESCHED" is not needed as a cond_resched() can safely be called at
the point this is returned, rather than returning it.
That leaves "OKAY" and "ABORT" which can simply by "true" and "false"
boolean values.

Internalizing the RESCHED case means the callers don't need to loop
themselves.  This simplify calling patterns.

Reviewed-by: Andreas Dilger <adilger@whamcloud.com>
Signed-off-by: NeilBrown <neilb@suse.com>
---
 drivers/staging/lustre/lustre/include/cl_object.h  |  7 ----
 drivers/staging/lustre/lustre/osc/osc_cache.c      | 39 ++++++++--------------
 .../staging/lustre/lustre/osc/osc_cl_internal.h    | 10 +++---
 drivers/staging/lustre/lustre/osc/osc_io.c         |  4 +--
 drivers/staging/lustre/lustre/osc/osc_lock.c       | 26 ++++++---------
 5 files changed, 32 insertions(+), 54 deletions(-)

diff --git a/drivers/staging/lustre/lustre/include/cl_object.h b/drivers/staging/lustre/lustre/include/cl_object.h
index 603281567219..41b32b7db515 100644
--- a/drivers/staging/lustre/lustre/include/cl_object.h
+++ b/drivers/staging/lustre/lustre/include/cl_object.h
@@ -2084,14 +2084,7 @@ static inline int cl_object_refc(struct cl_object *clob)
 /** \defgroup cl_page cl_page
  * @{
  */
-enum {
-	CLP_GANG_OKAY = 0,
-	CLP_GANG_RESCHED,
-	CLP_GANG_AGAIN,
-	CLP_GANG_ABORT
-};
 
-/* callback of cl_page_gang_lookup() */
 struct cl_page *cl_page_find(const struct lu_env *env, struct cl_object *obj,
 			     pgoff_t idx, struct page *vmpage,
 			     enum cl_page_type type);
diff --git a/drivers/staging/lustre/lustre/osc/osc_cache.c b/drivers/staging/lustre/lustre/osc/osc_cache.c
index 29fe8ef11af1..de3250a2d1ec 100644
--- a/drivers/staging/lustre/lustre/osc/osc_cache.c
+++ b/drivers/staging/lustre/lustre/osc/osc_cache.c
@@ -2989,18 +2989,14 @@ int osc_cache_writeback_range(const struct lu_env *env, struct osc_object *obj,
 /**
  * Returns a list of pages by a given [start, end] of \a obj.
  *
- * \param resched If not NULL, then we give up before hogging CPU for too
- * long and set *resched = 1, in that case caller should implement a retry
- * logic.
- *
  * Gang tree lookup (radix_tree_gang_lookup()) optimization is absolutely
  * crucial in the face of [offset, EOF] locks.
  *
  * Return at least one page in @queue unless there is no covered page.
  */
-int osc_page_gang_lookup(const struct lu_env *env, struct cl_io *io,
-			 struct osc_object *osc, pgoff_t start, pgoff_t end,
-			 osc_page_gang_cbt cb, void *cbdata)
+bool osc_page_gang_lookup(const struct lu_env *env, struct cl_io *io,
+			  struct osc_object *osc, pgoff_t start, pgoff_t end,
+			  osc_page_gang_cbt cb, void *cbdata)
 {
 	struct osc_page *ops;
 	void            **pvec;
@@ -3008,7 +3004,7 @@ int osc_page_gang_lookup(const struct lu_env *env, struct cl_io *io,
 	unsigned int    nr;
 	unsigned int    i;
 	unsigned int    j;
-	int             res = CLP_GANG_OKAY;
+	bool            res = true;
 	bool            tree_lock = true;
 
 	idx = start;
@@ -3054,7 +3050,7 @@ int osc_page_gang_lookup(const struct lu_env *env, struct cl_io *io,
 
 		for (i = 0; i < j; ++i) {
 			ops = pvec[i];
-			if (res == CLP_GANG_OKAY)
+			if (res)
 				res = (*cb)(env, io, ops, cbdata);
 
 			page = ops->ops_cl.cpl_page;
@@ -3064,10 +3060,10 @@ int osc_page_gang_lookup(const struct lu_env *env, struct cl_io *io,
 		if (nr < OTI_PVEC_SIZE || end_of_region)
 			break;
 
-		if (res == CLP_GANG_OKAY && need_resched())
-			res = CLP_GANG_RESCHED;
-		if (res != CLP_GANG_OKAY)
+		if (!res)
 			break;
+		if (need_resched())
+			cond_resched();
 
 		spin_lock(&osc->oo_tree_lock);
 		tree_lock = true;
@@ -3080,7 +3076,7 @@ int osc_page_gang_lookup(const struct lu_env *env, struct cl_io *io,
 /**
  * Check if page @page is covered by an extra lock or discard it.
  */
-static int check_and_discard_cb(const struct lu_env *env, struct cl_io *io,
+static bool check_and_discard_cb(const struct lu_env *env, struct cl_io *io,
 				struct osc_page *ops, void *cbdata)
 {
 	struct osc_thread_info *info = osc_env_info(env);
@@ -3116,10 +3112,10 @@ static int check_and_discard_cb(const struct lu_env *env, struct cl_io *io,
 	}
 
 	info->oti_next_index = index + 1;
-	return CLP_GANG_OKAY;
+	return true;
 }
 
-static int discard_cb(const struct lu_env *env, struct cl_io *io,
+static bool discard_cb(const struct lu_env *env, struct cl_io *io,
 		      struct osc_page *ops, void *cbdata)
 {
 	struct osc_thread_info *info = osc_env_info(env);
@@ -3140,7 +3136,7 @@ static int discard_cb(const struct lu_env *env, struct cl_io *io,
 		LASSERT(page->cp_state == CPS_FREEING);
 	}
 
-	return CLP_GANG_OKAY;
+	return true;
 }
 
 /**
@@ -3157,7 +3153,6 @@ int osc_lock_discard_pages(const struct lu_env *env, struct osc_object *osc,
 	struct osc_thread_info *info = osc_env_info(env);
 	struct cl_io *io = &info->oti_io;
 	osc_page_gang_cbt cb;
-	int res;
 	int result;
 
 	io->ci_obj = cl_object_top(osc2cl(osc));
@@ -3169,15 +3164,9 @@ int osc_lock_discard_pages(const struct lu_env *env, struct osc_object *osc,
 	cb = discard ? discard_cb : check_and_discard_cb;
 	info->oti_fn_index = start;
 	info->oti_next_index = start;
-	do {
-		res = osc_page_gang_lookup(env, io, osc,
-					   info->oti_next_index, end, cb, osc);
-		if (info->oti_next_index > end)
-			break;
 
-		if (res == CLP_GANG_RESCHED)
-			cond_resched();
-	} while (res != CLP_GANG_OKAY);
+	osc_page_gang_lookup(env, io, osc,
+			     info->oti_next_index, end, cb, osc);
 out:
 	cl_io_fini(env, io);
 	return result;
diff --git a/drivers/staging/lustre/lustre/osc/osc_cl_internal.h b/drivers/staging/lustre/lustre/osc/osc_cl_internal.h
index 3af096e0dbdd..c0f58f41513f 100644
--- a/drivers/staging/lustre/lustre/osc/osc_cl_internal.h
+++ b/drivers/staging/lustre/lustre/osc/osc_cl_internal.h
@@ -660,11 +660,11 @@ void osc_extent_release(const struct lu_env *env, struct osc_extent *ext);
 int osc_lock_discard_pages(const struct lu_env *env, struct osc_object *osc,
 			   pgoff_t start, pgoff_t end, bool discard_pages);
 
-typedef int (*osc_page_gang_cbt)(const struct lu_env *, struct cl_io *,
-				 struct osc_page *, void *);
-int osc_page_gang_lookup(const struct lu_env *env, struct cl_io *io,
-			 struct osc_object *osc, pgoff_t start, pgoff_t end,
-			 osc_page_gang_cbt cb, void *cbdata);
+typedef bool (*osc_page_gang_cbt)(const struct lu_env *, struct cl_io *,
+				  struct osc_page *, void *);
+bool osc_page_gang_lookup(const struct lu_env *env, struct cl_io *io,
+			  struct osc_object *osc, pgoff_t start, pgoff_t end,
+			  osc_page_gang_cbt cb, void *cbdata);
 /* @} osc */
 
 #endif /* OSC_CL_INTERNAL_H */
diff --git a/drivers/staging/lustre/lustre/osc/osc_io.c b/drivers/staging/lustre/lustre/osc/osc_io.c
index 8cd0813fb4bf..0a7bfe2d4059 100644
--- a/drivers/staging/lustre/lustre/osc/osc_io.c
+++ b/drivers/staging/lustre/lustre/osc/osc_io.c
@@ -449,7 +449,7 @@ static int osc_async_upcall(void *a, int rc)
 /**
  * Checks that there are no pages being written in the extent being truncated.
  */
-static int trunc_check_cb(const struct lu_env *env, struct cl_io *io,
+static bool trunc_check_cb(const struct lu_env *env, struct cl_io *io,
 			  struct osc_page *ops, void *cbdata)
 {
 	struct cl_page *page = ops->ops_cl.cpl_page;
@@ -466,7 +466,7 @@ static int trunc_check_cb(const struct lu_env *env, struct cl_io *io,
 		CDEBUG(D_CACHE, "page %p index %lu locked for %d.\n",
 		       ops, osc_index(ops), oap->oap_cmd & OBD_BRW_RWMASK);
 
-	return CLP_GANG_OKAY;
+	return true;
 }
 
 static void osc_trunc_check(const struct lu_env *env, struct cl_io *io,
diff --git a/drivers/staging/lustre/lustre/osc/osc_lock.c b/drivers/staging/lustre/lustre/osc/osc_lock.c
index 01b6bf7df3b7..1781243f1c66 100644
--- a/drivers/staging/lustre/lustre/osc/osc_lock.c
+++ b/drivers/staging/lustre/lustre/osc/osc_lock.c
@@ -630,18 +630,18 @@ static int osc_ldlm_glimpse_ast(struct ldlm_lock *dlmlock, void *data)
 	return result;
 }
 
-static int weigh_cb(const struct lu_env *env, struct cl_io *io,
-		    struct osc_page *ops, void *cbdata)
+static bool weigh_cb(const struct lu_env *env, struct cl_io *io,
+		     struct osc_page *ops, void *cbdata)
 {
 	struct cl_page *page = ops->ops_cl.cpl_page;
 
 	if (cl_page_is_vmlocked(env, page) ||
 	    PageDirty(page->cp_vmpage) || PageWriteback(page->cp_vmpage)
 	   )
-		return CLP_GANG_ABORT;
+		return false;
 
 	*(pgoff_t *)cbdata = osc_index(ops) + 1;
-	return CLP_GANG_OKAY;
+	return true;
 }
 
 static unsigned long osc_lock_weight(const struct lu_env *env,
@@ -660,19 +660,15 @@ static unsigned long osc_lock_weight(const struct lu_env *env,
 		return result;
 
 	page_index = cl_index(obj, extent->start);
-	do {
-		result = osc_page_gang_lookup(env, io, oscobj,
-					      page_index,
-					      cl_index(obj, extent->end),
-					      weigh_cb, (void *)&page_index);
-		if (result == CLP_GANG_ABORT)
-			break;
-		if (result == CLP_GANG_RESCHED)
-			cond_resched();
-	} while (result != CLP_GANG_OKAY);
+
+	if (!osc_page_gang_lookup(env, io, oscobj,
+				 page_index,
+				 cl_index(obj, extent->end),
+				 weigh_cb, (void *)&page_index))
+		result = 1;
 	cl_io_fini(env, io);
 
-	return result == CLP_GANG_ABORT ? 1 : 0;
+	return result;
 }
 
 /**
-- 
2.14.0.rc0.dirty

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <http://lists.lustre.org/pipermail/lustre-devel-lustre.org/attachments/20190111/448025ea/attachment.sig>

[lustre-devel] [PATCH 26/29] lustre: osc_cache: simplify osc_page_gang_lookup()

[Andreas Dilger]

On Jan 10, 2019, at 18:11, NeilBrown <neilb@suse.com> wrote:
> :
>> 
>> However, looking into the osc_page_gang_lookup() code more closely, I
>> see "end_of_region" would already be set in this case (it is just at
>> the start of the context in the above patch hunk) so CLP_GANG_RESCHED
>> should never be set in that case.  So it looks OK.
> 
> Oh good :-)
> Thanks.
> I love when a review includes what you saw as well as the "Reviewed-by".

Sometimes (IMHO) it points out that code "reads" in a misleading manner,
looking like it does one thing, but actually doing something else.  This
is mostly only obvious if you don't already know what the code is doing,
otherwise your own mental picture of the functionality guides you along.
I'm not really working on the CLIO code as it was developed for a project
that wanted to provide WinNT and MacOS code, and replaced the Lustre VFS
IO interface that I'd "grown up with".

>>> +static bool weigh_cb(const struct lu_env *env, struct cl_io *io,
>>> +		     struct osc_page *ops, void *cbdata)
>>> {
>>> 	struct cl_page *page = ops->ops_cl.cpl_page;
>>> 
>>> 	if (cl_page_is_vmlocked(env, page) ||
>>> 	    PageDirty(page->cp_vmpage) || PageWriteback(page->cp_vmpage)
>>> 	   )
>> 
>> This is a bit oddly formatted.  I see in our tree it looks like:
>> 
>>        if (cl_page_is_vmlocked(env, page) || PageDirty(page->cp_vmpage) ||
>>            PageWriteback(page->cp_vmpage))
>> 
>> which is more normal.
> 
> It has only been this way in OpenSFS since July this year.

I didn't know that when I was looking at the patch, just that the above
was looking strange with the lone closing parenthesis on the line.

> Commit b44b1ff8c7fc ("LU-10961 ldlm: don't cancel DoM locks before replay")
> made the change without any comment.  I guess we aren't up to that 2.11
> yet :-)

Sure, but I figured if you are changing the formatting anyway you may as
well make it consistent.

Cheers, Andreas
---
Andreas Dilger
Principal Lustre Architect
Whamcloud

[lustre-devel] [PATCH 26/29] lustre: osc_cache: simplify osc_page_gang_lookup()

[NeilBrown]

On Fri, Jan 11 2019, Andreas Dilger wrote:

> On Jan 10, 2019, at 18:11, NeilBrown <neilb@suse.com> wrote:
>> :
>>> 
>>> However, looking into the osc_page_gang_lookup() code more closely, I
>>> see "end_of_region" would already be set in this case (it is just at
>>> the start of the context in the above patch hunk) so CLP_GANG_RESCHED
>>> should never be set in that case.  So it looks OK.
>> 
>> Oh good :-)
>> Thanks.
>> I love when a review includes what you saw as well as the "Reviewed-by".
>
> Sometimes (IMHO) it points out that code "reads" in a misleading manner,
> looking like it does one thing, but actually doing something else.  This
> is mostly only obvious if you don't already know what the code is doing,
> otherwise your own mental picture of the functionality guides you along.
> I'm not really working on the CLIO code as it was developed for a project
> that wanted to provide WinNT and MacOS code, and replaced the Lustre VFS
> IO interface that I'd "grown up with".
>
>>>> +static bool weigh_cb(const struct lu_env *env, struct cl_io *io,
>>>> +		     struct osc_page *ops, void *cbdata)
>>>> {
>>>> 	struct cl_page *page = ops->ops_cl.cpl_page;
>>>> 
>>>> 	if (cl_page_is_vmlocked(env, page) ||
>>>> 	    PageDirty(page->cp_vmpage) || PageWriteback(page->cp_vmpage)
>>>> 	   )
>>> 
>>> This is a bit oddly formatted.  I see in our tree it looks like:
>>> 
>>>        if (cl_page_is_vmlocked(env, page) || PageDirty(page->cp_vmpage) ||
>>>            PageWriteback(page->cp_vmpage))
>>> 
>>> which is more normal.
>> 
>> It has only been this way in OpenSFS since July this year.
>
> I didn't know that when I was looking at the patch, just that the above
> was looking strange with the lone closing parenthesis on the line.
>
>> Commit b44b1ff8c7fc ("LU-10961 ldlm: don't cancel DoM locks before replay")
>> made the change without any comment.  I guess we aren't up to that 2.11
>> yet :-)
>
> Sure, but I figured if you are changing the formatting anyway you may as
> well make it consistent.

If I was, I probably would.  But I didn't.
i.e. this patch didn't change
        if (cl_page_is_vmlocked(env, page) ||
            PageDirty(page->cp_vmpage) || PageWriteback(page->cp_vmpage)
           )

so there is no case to be made that it should change it in a better way.
This patch changes the type of value returned by a callback, and makes
related changes that are a direct consequence of that.  It should do
nothing else.  Making unrelated changes in the same patch just makes the
patch harder to review.

So I'll leave this code as it is for now.

Thanks,
NeilBrown


>
> Cheers, Andreas
> ---
> Andreas Dilger
> Principal Lustre Architect
> Whamcloud
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <http://lists.lustre.org/pipermail/lustre-devel-lustre.org/attachments/20190130/0be047fe/attachment.sig>

[lustre-devel] [PATCH 29/29] lustre: centralize handling of PTLRPCD_SET

[NeilBrown]

Various places test if a given rqset is PTLRPCD_SET
and call either ptlrpcd_add_req() or ptlrpc_set_add_req()
depending on the result.

This can be unified by putting the test of PTLRPCD_SET in
ptlrpc_set_add_req(), and always calling that function.

This results in the being only one place that tests PTLRPCD_SET.

Signed-off-by: NeilBrown <neilb@suse.com>
---
 drivers/staging/lustre/lustre/include/lustre_net.h |    1 +
 drivers/staging/lustre/lustre/osc/osc_internal.h   |    2 -
 drivers/staging/lustre/lustre/osc/osc_request.c    |   27 ++++----------------
 drivers/staging/lustre/lustre/ptlrpc/client.c      |    5 ++++
 4 files changed, 11 insertions(+), 24 deletions(-)

diff --git a/drivers/staging/lustre/lustre/include/lustre_net.h b/drivers/staging/lustre/lustre/include/lustre_net.h
index 6d328b48a96b..e665f70e54a8 100644
--- a/drivers/staging/lustre/lustre/include/lustre_net.h
+++ b/drivers/staging/lustre/lustre/include/lustre_net.h
@@ -1832,6 +1832,7 @@ int ptlrpc_check_set(const struct lu_env *env, struct ptlrpc_request_set *set);
 int ptlrpc_set_wait(struct ptlrpc_request_set *);
 void ptlrpc_set_destroy(struct ptlrpc_request_set *);
 void ptlrpc_set_add_req(struct ptlrpc_request_set *, struct ptlrpc_request *);
+#define PTLRPCD_SET ((struct ptlrpc_request_set*)1)
 
 void ptlrpc_free_rq_pool(struct ptlrpc_request_pool *pool);
 int ptlrpc_add_rqs_to_pool(struct ptlrpc_request_pool *pool, int num_rq);
diff --git a/drivers/staging/lustre/lustre/osc/osc_internal.h b/drivers/staging/lustre/lustre/osc/osc_internal.h
index 586f0dfe3790..c61ef894177b 100644
--- a/drivers/staging/lustre/lustre/osc/osc_internal.h
+++ b/drivers/staging/lustre/lustre/osc/osc_internal.h
@@ -99,8 +99,6 @@ void osc_update_next_shrink(struct client_obd *cli);
  */
 #include <cl_object.h>
 
-extern struct ptlrpc_request_set *PTLRPCD_SET;
-
 typedef int (*osc_enqueue_upcall_f)(void *cookie, struct lustre_handle *lockh,
 				    int rc);
 
diff --git a/drivers/staging/lustre/lustre/osc/osc_request.c b/drivers/staging/lustre/lustre/osc/osc_request.c
index c2239c99a7b2..87cc8c72f6e6 100644
--- a/drivers/staging/lustre/lustre/osc/osc_request.c
+++ b/drivers/staging/lustre/lustre/osc/osc_request.c
@@ -266,10 +266,7 @@ int osc_setattr_async(struct obd_export *exp, struct obdo *oa,
 		sa->sa_upcall = upcall;
 		sa->sa_cookie = cookie;
 
-		if (rqset == PTLRPCD_SET)
-			ptlrpcd_add_req(req);
-		else
-			ptlrpc_set_add_req(rqset, req);
+		ptlrpc_set_add_req(rqset, req);
 	}
 
 	return 0;
@@ -354,10 +351,7 @@ int osc_ladvise_base(struct obd_export *exp, struct obdo *oa,
 	la->la_upcall = upcall;
 	la->la_cookie = cookie;
 
-	if (rqset == PTLRPCD_SET)
-		ptlrpcd_add_req(req);
-	else
-		ptlrpc_set_add_req(rqset, req);
+	ptlrpc_set_add_req(rqset, req);
 
 	return 0;
 }
@@ -450,10 +444,7 @@ int osc_punch_base(struct obd_export *exp, struct obdo *oa,
 	sa->sa_oa = oa;
 	sa->sa_upcall = upcall;
 	sa->sa_cookie = cookie;
-	if (rqset == PTLRPCD_SET)
-		ptlrpcd_add_req(req);
-	else
-		ptlrpc_set_add_req(rqset, req);
+	ptlrpc_set_add_req(rqset, req);
 
 	return 0;
 }
@@ -533,10 +524,7 @@ int osc_sync_base(struct osc_object *obj, struct obdo *oa,
 	fa->fa_upcall = upcall;
 	fa->fa_cookie = cookie;
 
-	if (rqset == PTLRPCD_SET)
-		ptlrpcd_add_req(req);
-	else
-		ptlrpc_set_add_req(rqset, req);
+	ptlrpc_set_add_req(rqset, req);
 
 	return 0;
 }
@@ -2148,8 +2136,6 @@ static int osc_enqueue_interpret(const struct lu_env *env,
 	return rc;
 }
 
-struct ptlrpc_request_set *PTLRPCD_SET = (void *)1;
-
 /* When enqueuing asynchronously, locks are not ordered, we can obtain a lock
  * from the 2nd OSC before a lock from the 1st one. This does not deadlock with
  * other synchronous requests, however keeping some locks and trying to obtain
@@ -2291,10 +2277,7 @@ int osc_enqueue_base(struct obd_export *exp, struct ldlm_res_id *res_id,
 
 			req->rq_interpret_reply =
 				(ptlrpc_interpterer_t)osc_enqueue_interpret;
-			if (rqset == PTLRPCD_SET)
-				ptlrpcd_add_req(req);
-			else
-				ptlrpc_set_add_req(rqset, req);
+			ptlrpc_set_add_req(rqset, req);
 		} else if (intent) {
 			ptlrpc_req_finished(req);
 		}
diff --git a/drivers/staging/lustre/lustre/ptlrpc/client.c b/drivers/staging/lustre/lustre/ptlrpc/client.c
index f90a3eef5daf..110bb5d8f767 100644
--- a/drivers/staging/lustre/lustre/ptlrpc/client.c
+++ b/drivers/staging/lustre/lustre/ptlrpc/client.c
@@ -1048,6 +1048,11 @@ EXPORT_SYMBOL(ptlrpc_set_destroy);
 void ptlrpc_set_add_req(struct ptlrpc_request_set *set,
 			struct ptlrpc_request *req)
 {
+	if (set == PTLRPCD_SET) {
+		ptlrpcd_add_req(req);
+		return;
+	}
+
 	LASSERT(list_empty(&req->rq_set_chain));
 
 	/* The set takes over the caller's request reference */

[lustre-devel] [PATCH 29/29] lustre: centralize handling of PTLRPCD_SET

[Andreas Dilger]

On Jan 8, 2019, at 23:24, NeilBrown <neilb@suse.com> wrote:
> 
> Various places test if a given rqset is PTLRPCD_SET
> and call either ptlrpcd_add_req() or ptlrpc_set_add_req()
> depending on the result.
> 
> This can be unified by putting the test of PTLRPCD_SET in
> ptlrpc_set_add_req(), and always calling that function.
> 
> This results in the being only one place that tests PTLRPCD_SET.
> 
> Signed-off-by: NeilBrown <neilb@suse.com>
> ---
> 
> diff --git a/drivers/staging/lustre/lustre/include/lustre_net.h b/drivers/staging/lustre/lustre/include/lustre_net.h
> index 6d328b48a96b..e665f70e54a8 100644
> --- a/drivers/staging/lustre/lustre/include/lustre_net.h
> +++ b/drivers/staging/lustre/lustre/include/lustre_net.h
> @@ -1832,6 +1832,7 @@ int ptlrpc_check_set(const struct lu_env *env, struct ptlrpc_request_set *set);
> int ptlrpc_set_wait(struct ptlrpc_request_set *);
> void ptlrpc_set_destroy(struct ptlrpc_request_set *);
> void ptlrpc_set_add_req(struct ptlrpc_request_set *, struct ptlrpc_request *);
> +#define PTLRPCD_SET ((struct ptlrpc_request_set*)1)

(style) space before that '*'

Nice cleanup otherwise.  Definitely removes a bunch of boilerplate code.

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

Cheers, Andreas
---
Andreas Dilger
Principal Lustre Architect
Whamcloud