diff mbox series

[v1,2/2] NFSD: Re-arrange file_close_inode tracepoints

Message ID 166750697425.1646.11770177003223505657.stgit@klimt.1015granger.net (mailing list archive)
State New, archived
Headers show
Series A couple of tracepoint updates | expand

Commit Message

Chuck Lever Nov. 3, 2022, 8:22 p.m. UTC 
Now that we have trace_nfsd_file_closing, all we really need to
capture is when an external caller has requested a close/sync.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfsd/filecache.c |   17 ++++++-----------
 fs/nfsd/trace.h     |   45 ++++++++++++++++-----------------------------
 2 files changed, 22 insertions(+), 40 deletions(-)

Comments

Jeff Layton Nov. 4, 2022, 12:44 p.m. UTC | #1 
On Thu, 2022-11-03 at 16:22 -0400, Chuck Lever wrote:
> Now that we have trace_nfsd_file_closing, all we really need to
> capture is when an external caller has requested a close/sync.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  fs/nfsd/filecache.c |   17 ++++++-----------
>  fs/nfsd/trace.h     |   45 ++++++++++++++++-----------------------------
>  2 files changed, 22 insertions(+), 40 deletions(-)
> 
> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> index cf1a8f1d1349..7be62af4bfb7 100644
> --- a/fs/nfsd/filecache.c
> +++ b/fs/nfsd/filecache.c
> @@ -706,14 +706,13 @@ static struct shrinker	nfsd_file_shrinker = {
>   * The nfsd_file objects on the list will be unhashed, and each will have a
>   * reference taken.
>   */
> -static unsigned int
> +static void
>  __nfsd_file_close_inode(struct inode *inode, struct list_head *dispose)
>  {
>  	struct nfsd_file_lookup_key key = {
>  		.type	= NFSD_FILE_KEY_INODE,
>  		.inode	= inode,
>  	};
> -	unsigned int count = 0;
>  	struct nfsd_file *nf;
>  
>  	rcu_read_lock();
> @@ -723,11 +722,9 @@ __nfsd_file_close_inode(struct inode *inode, struct list_head *dispose)
>  		if (!nf)
>  			break;
>  
> -		if (nfsd_file_unhash_and_queue(nf, dispose))
> -			count++;
> +		nfsd_file_unhash_and_queue(nf, dispose);
>  	} while (1);
>  	rcu_read_unlock();
> -	return count;
>  }
>  
>  /**
> @@ -742,11 +739,9 @@ static void
>  nfsd_file_close_inode(struct inode *inode)
>  {
>  	struct nfsd_file *nf, *tmp;
> -	unsigned int count;
>  	LIST_HEAD(dispose);
>  
> -	count = __nfsd_file_close_inode(inode, &dispose);
> -	trace_nfsd_file_close_inode(inode, count);
> +	__nfsd_file_close_inode(inode, &dispose);
>  	list_for_each_entry_safe(nf, tmp, &dispose, nf_lru) {
>  		trace_nfsd_file_closing(nf);
>  		if (!refcount_dec_and_test(&nf->nf_ref))
> @@ -765,11 +760,11 @@ void
>  nfsd_file_close_inode_sync(struct inode *inode)
>  {
>  	struct nfsd_file *nf;
> -	unsigned int count;
>  	LIST_HEAD(dispose);
>  
> -	count = __nfsd_file_close_inode(inode, &dispose);
> -	trace_nfsd_file_close_inode(inode, count);
> +	trace_nfsd_file_close(inode);

With this change we lose the count of entries on the list. That info is
not particularly helpful, but maybe we ought to consider a tracepoint in
unhash_and_queue that records whether a file we found in the hash got
queued? It might be nice to have a way to detect cases where we close a
nfsd_file but the refcount was >1 or 0, so we don't end up queueing it
to the list.

> +
> +	__nfsd_file_close_inode(inode, &dispose);
>  	while (!list_empty(&dispose)) {
>  		nf = list_first_entry(&dispose, struct nfsd_file, nf_lru);
>  		list_del_init(&nf->nf_lru);
> diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h
> index e41007807b7e..ef01ecd3eec6 100644
> --- a/fs/nfsd/trace.h
> +++ b/fs/nfsd/trace.h
> @@ -1099,35 +1099,6 @@ TRACE_EVENT(nfsd_file_open,
>  		__entry->nf_file)
>  )
>  
> -DECLARE_EVENT_CLASS(nfsd_file_search_class,
> -	TP_PROTO(
> -		const struct inode *inode,
> -		unsigned int count
> -	),
> -	TP_ARGS(inode, count),
> -	TP_STRUCT__entry(
> -		__field(const struct inode *, inode)
> -		__field(unsigned int, count)
> -	),
> -	TP_fast_assign(
> -		__entry->inode = inode;
> -		__entry->count = count;
> -	),
> -	TP_printk("inode=%p count=%u",
> -		__entry->inode, __entry->count)
> -);
> -
> -#define DEFINE_NFSD_FILE_SEARCH_EVENT(name)				\
> -DEFINE_EVENT(nfsd_file_search_class, name,				\
> -	TP_PROTO(							\
> -		const struct inode *inode,				\
> -		unsigned int count					\
> -	),								\
> -	TP_ARGS(inode, count))
> -
> -DEFINE_NFSD_FILE_SEARCH_EVENT(nfsd_file_close_inode_sync);
> -DEFINE_NFSD_FILE_SEARCH_EVENT(nfsd_file_close_inode);
> -
>  TRACE_EVENT(nfsd_file_is_cached,
>  	TP_PROTO(
>  		const struct inode *inode,
> @@ -1238,6 +1209,22 @@ DEFINE_EVENT(nfsd_file_lruwalk_class, name,				\
>  DEFINE_NFSD_FILE_LRUWALK_EVENT(nfsd_file_gc_removed);
>  DEFINE_NFSD_FILE_LRUWALK_EVENT(nfsd_file_shrinker_removed);
>  
> +TRACE_EVENT(nfsd_file_close,
> +	TP_PROTO(
> +		const struct inode *inode
> +	),
> +	TP_ARGS(inode),
> +	TP_STRUCT__entry(
> +		__field(const void *, inode)
> +	),
> +	TP_fast_assign(
> +		__entry->inode = inode;
> +	),
> +	TP_printk("inode=%p",
> +		__entry->inode
> +	)
> +);
> +
>  TRACE_EVENT(nfsd_file_fsync,
>  	TP_PROTO(
>  		const struct nfsd_file *nf,
> 
>
Chuck Lever Nov. 4, 2022, 1:35 p.m. UTC | #2 
> On Nov 4, 2022, at 8:44 AM, Jeff Layton <jlayton@poochiereds.net> wrote:
> 
> On Thu, 2022-11-03 at 16:22 -0400, Chuck Lever wrote:
>> Now that we have trace_nfsd_file_closing, all we really need to
>> capture is when an external caller has requested a close/sync.
>> 
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>> fs/nfsd/filecache.c |   17 ++++++-----------
>> fs/nfsd/trace.h     |   45 ++++++++++++++++-----------------------------
>> 2 files changed, 22 insertions(+), 40 deletions(-)
>> 
>> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
>> index cf1a8f1d1349..7be62af4bfb7 100644
>> --- a/fs/nfsd/filecache.c
>> +++ b/fs/nfsd/filecache.c
>> @@ -706,14 +706,13 @@ static struct shrinker	nfsd_file_shrinker = {
>>  * The nfsd_file objects on the list will be unhashed, and each will have a
>>  * reference taken.
>>  */
>> -static unsigned int
>> +static void
>> __nfsd_file_close_inode(struct inode *inode, struct list_head *dispose)
>> {
>> 	struct nfsd_file_lookup_key key = {
>> 		.type	= NFSD_FILE_KEY_INODE,
>> 		.inode	= inode,
>> 	};
>> -	unsigned int count = 0;
>> 	struct nfsd_file *nf;
>> 
>> 	rcu_read_lock();
>> @@ -723,11 +722,9 @@ __nfsd_file_close_inode(struct inode *inode, struct list_head *dispose)
>> 		if (!nf)
>> 			break;
>> 
>> -		if (nfsd_file_unhash_and_queue(nf, dispose))
>> -			count++;
>> +		nfsd_file_unhash_and_queue(nf, dispose);
>> 	} while (1);
>> 	rcu_read_unlock();
>> -	return count;
>> }
>> 
>> /**
>> @@ -742,11 +739,9 @@ static void
>> nfsd_file_close_inode(struct inode *inode)
>> {
>> 	struct nfsd_file *nf, *tmp;
>> -	unsigned int count;
>> 	LIST_HEAD(dispose);
>> 
>> -	count = __nfsd_file_close_inode(inode, &dispose);
>> -	trace_nfsd_file_close_inode(inode, count);
>> +	__nfsd_file_close_inode(inode, &dispose);
>> 	list_for_each_entry_safe(nf, tmp, &dispose, nf_lru) {
>> 		trace_nfsd_file_closing(nf);
>> 		if (!refcount_dec_and_test(&nf->nf_ref))
>> @@ -765,11 +760,11 @@ void
>> nfsd_file_close_inode_sync(struct inode *inode)
>> {
>> 	struct nfsd_file *nf;
>> -	unsigned int count;
>> 	LIST_HEAD(dispose);
>> 
>> -	count = __nfsd_file_close_inode(inode, &dispose);
>> -	trace_nfsd_file_close_inode(inode, count);
>> +	trace_nfsd_file_close(inode);
> 
> With this change we lose the count of entries on the list. That info is
> not particularly helpful, but maybe we ought to consider a tracepoint in
> unhash_and_queue that records whether a file we found in the hash got
> queued? It might be nice to have a way to detect cases where we close a
> nfsd_file but the refcount was >1 or 0, so we don't end up queueing it
> to the list.

Would that be an exception case, or an error? I'm assuming the former.
No objection if you'd like to add that as part of a follow-on series.


>> +
>> +	__nfsd_file_close_inode(inode, &dispose);
>> 	while (!list_empty(&dispose)) {
>> 		nf = list_first_entry(&dispose, struct nfsd_file, nf_lru);
>> 		list_del_init(&nf->nf_lru);
>> diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h
>> index e41007807b7e..ef01ecd3eec6 100644
>> --- a/fs/nfsd/trace.h
>> +++ b/fs/nfsd/trace.h
>> @@ -1099,35 +1099,6 @@ TRACE_EVENT(nfsd_file_open,
>> 		__entry->nf_file)
>> )
>> 
>> -DECLARE_EVENT_CLASS(nfsd_file_search_class,
>> -	TP_PROTO(
>> -		const struct inode *inode,
>> -		unsigned int count
>> -	),
>> -	TP_ARGS(inode, count),
>> -	TP_STRUCT__entry(
>> -		__field(const struct inode *, inode)
>> -		__field(unsigned int, count)
>> -	),
>> -	TP_fast_assign(
>> -		__entry->inode = inode;
>> -		__entry->count = count;
>> -	),
>> -	TP_printk("inode=%p count=%u",
>> -		__entry->inode, __entry->count)
>> -);
>> -
>> -#define DEFINE_NFSD_FILE_SEARCH_EVENT(name)				\
>> -DEFINE_EVENT(nfsd_file_search_class, name,				\
>> -	TP_PROTO(							\
>> -		const struct inode *inode,				\
>> -		unsigned int count					\
>> -	),								\
>> -	TP_ARGS(inode, count))
>> -
>> -DEFINE_NFSD_FILE_SEARCH_EVENT(nfsd_file_close_inode_sync);
>> -DEFINE_NFSD_FILE_SEARCH_EVENT(nfsd_file_close_inode);
>> -
>> TRACE_EVENT(nfsd_file_is_cached,
>> 	TP_PROTO(
>> 		const struct inode *inode,
>> @@ -1238,6 +1209,22 @@ DEFINE_EVENT(nfsd_file_lruwalk_class, name,				\
>> DEFINE_NFSD_FILE_LRUWALK_EVENT(nfsd_file_gc_removed);
>> DEFINE_NFSD_FILE_LRUWALK_EVENT(nfsd_file_shrinker_removed);
>> 
>> +TRACE_EVENT(nfsd_file_close,
>> +	TP_PROTO(
>> +		const struct inode *inode
>> +	),
>> +	TP_ARGS(inode),
>> +	TP_STRUCT__entry(
>> +		__field(const void *, inode)
>> +	),
>> +	TP_fast_assign(
>> +		__entry->inode = inode;
>> +	),
>> +	TP_printk("inode=%p",
>> +		__entry->inode
>> +	)
>> +);
>> +
>> TRACE_EVENT(nfsd_file_fsync,
>> 	TP_PROTO(
>> 		const struct nfsd_file *nf,
>> 
>> 
> 
> -- 
> Jeff Layton <jlayton@poochiereds.net>

--
Chuck Lever
diff mbox series

Patch

diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
index cf1a8f1d1349..7be62af4bfb7 100644
--- a/fs/nfsd/filecache.c
+++ b/fs/nfsd/filecache.c
@@ -706,14 +706,13 @@  static struct shrinker	nfsd_file_shrinker = {
  * The nfsd_file objects on the list will be unhashed, and each will have a
  * reference taken.
  */
-static unsigned int
+static void
 __nfsd_file_close_inode(struct inode *inode, struct list_head *dispose)
 {
 	struct nfsd_file_lookup_key key = {
 		.type	= NFSD_FILE_KEY_INODE,
 		.inode	= inode,
 	};
-	unsigned int count = 0;
 	struct nfsd_file *nf;
 
 	rcu_read_lock();
@@ -723,11 +722,9 @@  __nfsd_file_close_inode(struct inode *inode, struct list_head *dispose)
 		if (!nf)
 			break;
 
-		if (nfsd_file_unhash_and_queue(nf, dispose))
-			count++;
+		nfsd_file_unhash_and_queue(nf, dispose);
 	} while (1);
 	rcu_read_unlock();
-	return count;
 }
 
 /**
@@ -742,11 +739,9 @@  static void
 nfsd_file_close_inode(struct inode *inode)
 {
 	struct nfsd_file *nf, *tmp;
-	unsigned int count;
 	LIST_HEAD(dispose);
 
-	count = __nfsd_file_close_inode(inode, &dispose);
-	trace_nfsd_file_close_inode(inode, count);
+	__nfsd_file_close_inode(inode, &dispose);
 	list_for_each_entry_safe(nf, tmp, &dispose, nf_lru) {
 		trace_nfsd_file_closing(nf);
 		if (!refcount_dec_and_test(&nf->nf_ref))
@@ -765,11 +760,11 @@  void
 nfsd_file_close_inode_sync(struct inode *inode)
 {
 	struct nfsd_file *nf;
-	unsigned int count;
 	LIST_HEAD(dispose);
 
-	count = __nfsd_file_close_inode(inode, &dispose);
-	trace_nfsd_file_close_inode(inode, count);
+	trace_nfsd_file_close(inode);
+
+	__nfsd_file_close_inode(inode, &dispose);
 	while (!list_empty(&dispose)) {
 		nf = list_first_entry(&dispose, struct nfsd_file, nf_lru);
 		list_del_init(&nf->nf_lru);
diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h
index e41007807b7e..ef01ecd3eec6 100644
--- a/fs/nfsd/trace.h
+++ b/fs/nfsd/trace.h
@@ -1099,35 +1099,6 @@  TRACE_EVENT(nfsd_file_open,
 		__entry->nf_file)
 )
 
-DECLARE_EVENT_CLASS(nfsd_file_search_class,
-	TP_PROTO(
-		const struct inode *inode,
-		unsigned int count
-	),
-	TP_ARGS(inode, count),
-	TP_STRUCT__entry(
-		__field(const struct inode *, inode)
-		__field(unsigned int, count)
-	),
-	TP_fast_assign(
-		__entry->inode = inode;
-		__entry->count = count;
-	),
-	TP_printk("inode=%p count=%u",
-		__entry->inode, __entry->count)
-);
-
-#define DEFINE_NFSD_FILE_SEARCH_EVENT(name)				\
-DEFINE_EVENT(nfsd_file_search_class, name,				\
-	TP_PROTO(							\
-		const struct inode *inode,				\
-		unsigned int count					\
-	),								\
-	TP_ARGS(inode, count))
-
-DEFINE_NFSD_FILE_SEARCH_EVENT(nfsd_file_close_inode_sync);
-DEFINE_NFSD_FILE_SEARCH_EVENT(nfsd_file_close_inode);
-
 TRACE_EVENT(nfsd_file_is_cached,
 	TP_PROTO(
 		const struct inode *inode,
@@ -1238,6 +1209,22 @@  DEFINE_EVENT(nfsd_file_lruwalk_class, name,				\
 DEFINE_NFSD_FILE_LRUWALK_EVENT(nfsd_file_gc_removed);
 DEFINE_NFSD_FILE_LRUWALK_EVENT(nfsd_file_shrinker_removed);
 
+TRACE_EVENT(nfsd_file_close,
+	TP_PROTO(
+		const struct inode *inode
+	),
+	TP_ARGS(inode),
+	TP_STRUCT__entry(
+		__field(const void *, inode)
+	),
+	TP_fast_assign(
+		__entry->inode = inode;
+	),
+	TP_printk("inode=%p",
+		__entry->inode
+	)
+);
+
 TRACE_EVENT(nfsd_file_fsync,
 	TP_PROTO(
 		const struct nfsd_file *nf,