fs/fuse kio: skip iostat count for unknown req types

Submitted by Pavel Butsykin on May 31, 2019, 9:23 a.m.

Details

Message ID 20190531092344.31088-1-pbutsykin@virtuozzo.com
State New
Series "fs/fuse kio: skip iostat count for unknown req types"
Headers show

Commit Message

Pavel Butsykin May 31, 2019, 9:23 a.m.
There are still some number of response req types:
PCS_CS_WRITE_ZERO_RESP
PCS_CS_WRITE_HOLE_RESP
PCS_CS_REPLICATEX_RESP
PCS_CS_FIEMAP_RESP
PCS_CS_MAP_PROP_RESP

It makes no sense to count statistics for such requests, so for them the stat
count will simply be skipped.

Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
---
 fs/fuse/kio/pcs/fuse_stat.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Patch hide | download patch | download mbox

diff --git a/fs/fuse/kio/pcs/fuse_stat.c b/fs/fuse/kio/pcs/fuse_stat.c
index 06be3c75240c..31ff73d6c3dc 100644
--- a/fs/fuse/kio/pcs/fuse_stat.c
+++ b/fs/fuse/kio/pcs/fuse_stat.c
@@ -654,7 +654,6 @@  struct fuse_val_stat *req_stat_entry(struct pcs_fuse_io_stat *io, u32 type)
 		default:
 			break;
 	}
-	WARN_ON_ONCE(1);
 	return NULL;
 }
 
@@ -663,9 +662,11 @@  static void fuse_iostat_count(struct pcs_fuse_io_stat_sync *iostat,
 {
 	struct fuse_val_stat *se;
 
-	write_seqlock(&iostat->seqlock);
 	se = req_stat_entry(iostat->CURR(iostat), type);
-	BUG_ON(!se);
+	if (!se)
+		return;
+
+	write_seqlock(&iostat->seqlock);
 	fuse_val_stat_update(se, size);
 	write_sequnlock(&iostat->seqlock);
 }

Comments

Pavel Butsykin May 31, 2019, 9:24 a.m.
#VSTOR-23620

On 31.05.2019 12:23, Pavel Butsykin wrote:
> There are still some number of response req types:
> PCS_CS_WRITE_ZERO_RESP
> PCS_CS_WRITE_HOLE_RESP
> PCS_CS_REPLICATEX_RESP
> PCS_CS_FIEMAP_RESP
> PCS_CS_MAP_PROP_RESP
> 
> It makes no sense to count statistics for such requests, so for them the stat
> count will simply be skipped.
> 
> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
> ---
>   fs/fuse/kio/pcs/fuse_stat.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/fuse/kio/pcs/fuse_stat.c b/fs/fuse/kio/pcs/fuse_stat.c
> index 06be3c75240c..31ff73d6c3dc 100644
> --- a/fs/fuse/kio/pcs/fuse_stat.c
> +++ b/fs/fuse/kio/pcs/fuse_stat.c
> @@ -654,7 +654,6 @@ struct fuse_val_stat *req_stat_entry(struct pcs_fuse_io_stat *io, u32 type)
>   		default:
>   			break;
>   	}
> -	WARN_ON_ONCE(1);
>   	return NULL;
>   }
>   
> @@ -663,9 +662,11 @@ static void fuse_iostat_count(struct pcs_fuse_io_stat_sync *iostat,
>   {
>   	struct fuse_val_stat *se;
>   
> -	write_seqlock(&iostat->seqlock);
>   	se = req_stat_entry(iostat->CURR(iostat), type);
> -	BUG_ON(!se);
> +	if (!se)
> +		return;
> +
> +	write_seqlock(&iostat->seqlock);
>   	fuse_val_stat_update(se, size);
>   	write_sequnlock(&iostat->seqlock);
>   }
>
Kirill Tkhai May 31, 2019, 4:06 p.m.
On 31.05.2019 12:23, Pavel Butsykin wrote:
> There are still some number of response req types:
> PCS_CS_WRITE_ZERO_RESP
> PCS_CS_WRITE_HOLE_RESP
> PCS_CS_REPLICATEX_RESP
> PCS_CS_FIEMAP_RESP
> PCS_CS_MAP_PROP_RESP
> 
> It makes no sense to count statistics for such requests, so for them the stat
> count will simply be skipped.
> 
> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
> ---
>  fs/fuse/kio/pcs/fuse_stat.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/fuse/kio/pcs/fuse_stat.c b/fs/fuse/kio/pcs/fuse_stat.c
> index 06be3c75240c..31ff73d6c3dc 100644
> --- a/fs/fuse/kio/pcs/fuse_stat.c
> +++ b/fs/fuse/kio/pcs/fuse_stat.c
> @@ -654,7 +654,6 @@ struct fuse_val_stat *req_stat_entry(struct pcs_fuse_io_stat *io, u32 type)
>  		default:
>  			break;
>  	}
> -	WARN_ON_ONCE(1);
>  	return NULL;
>  }
>  
> @@ -663,9 +662,11 @@ static void fuse_iostat_count(struct pcs_fuse_io_stat_sync *iostat,
>  {
>  	struct fuse_val_stat *se;
>  
> -	write_seqlock(&iostat->seqlock);
>  	se = req_stat_entry(iostat->CURR(iostat), type);

Shouldn't iostat->CURR(iostat) be obtained under seqlock?
Pavel Butsykin June 3, 2019, 6:41 a.m.
On 31.05.2019 19:06, Kirill Tkhai wrote:
> On 31.05.2019 12:23, Pavel Butsykin wrote:
>> There are still some number of response req types:
>> PCS_CS_WRITE_ZERO_RESP
>> PCS_CS_WRITE_HOLE_RESP
>> PCS_CS_REPLICATEX_RESP
>> PCS_CS_FIEMAP_RESP
>> PCS_CS_MAP_PROP_RESP
>>
>> It makes no sense to count statistics for such requests, so for them the stat
>> count will simply be skipped.
>>
>> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
>> ---
>>   fs/fuse/kio/pcs/fuse_stat.c | 7 ++++---
>>   1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/fuse/kio/pcs/fuse_stat.c b/fs/fuse/kio/pcs/fuse_stat.c
>> index 06be3c75240c..31ff73d6c3dc 100644
>> --- a/fs/fuse/kio/pcs/fuse_stat.c
>> +++ b/fs/fuse/kio/pcs/fuse_stat.c
>> @@ -654,7 +654,6 @@ struct fuse_val_stat *req_stat_entry(struct pcs_fuse_io_stat *io, u32 type)
>>   		default:
>>   			break;
>>   	}
>> -	WARN_ON_ONCE(1);
>>   	return NULL;
>>   }
>>   
>> @@ -663,9 +662,11 @@ static void fuse_iostat_count(struct pcs_fuse_io_stat_sync *iostat,
>>   {
>>   	struct fuse_val_stat *se;
>>   
>> -	write_seqlock(&iostat->seqlock);
>>   	se = req_stat_entry(iostat->CURR(iostat), type);
> 
> Shouldn't iostat->CURR(iostat) be obtained under seqlock?
> 

Yep, iostat->CURR(iostat) should be under seqlock, thanks for fixup!
Kirill Tkhai June 3, 2019, 7:14 a.m.
On 03.06.2019 09:41, Pavel Butsykin wrote:
> 
> 
> On 31.05.2019 19:06, Kirill Tkhai wrote:
>> On 31.05.2019 12:23, Pavel Butsykin wrote:
>>> There are still some number of response req types:
>>> PCS_CS_WRITE_ZERO_RESP
>>> PCS_CS_WRITE_HOLE_RESP
>>> PCS_CS_REPLICATEX_RESP
>>> PCS_CS_FIEMAP_RESP
>>> PCS_CS_MAP_PROP_RESP
>>>
>>> It makes no sense to count statistics for such requests, so for them the stat
>>> count will simply be skipped.
>>>
>>> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
>>> ---
>>>   fs/fuse/kio/pcs/fuse_stat.c | 7 ++++---
>>>   1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/fs/fuse/kio/pcs/fuse_stat.c b/fs/fuse/kio/pcs/fuse_stat.c
>>> index 06be3c75240c..31ff73d6c3dc 100644
>>> --- a/fs/fuse/kio/pcs/fuse_stat.c
>>> +++ b/fs/fuse/kio/pcs/fuse_stat.c
>>> @@ -654,7 +654,6 @@ struct fuse_val_stat *req_stat_entry(struct pcs_fuse_io_stat *io, u32 type)
>>>   		default:
>>>   			break;
>>>   	}
>>> -	WARN_ON_ONCE(1);
>>>   	return NULL;
>>>   }
>>>   
>>> @@ -663,9 +662,11 @@ static void fuse_iostat_count(struct pcs_fuse_io_stat_sync *iostat,
>>>   {
>>>   	struct fuse_val_stat *se;
>>>   
>>> -	write_seqlock(&iostat->seqlock);
>>>   	se = req_stat_entry(iostat->CURR(iostat), type);
>>
>> Shouldn't iostat->CURR(iostat) be obtained under seqlock?
>>
> 
> Yep, iostat->CURR(iostat) should be under seqlock, thanks for fixup!

I think it may be useful to add some lockdep check into CURR and other macros.

Kirill
Pavel Butsykin June 3, 2019, 7:18 a.m.
On 03.06.2019 10:14, Kirill Tkhai wrote:
> On 03.06.2019 09:41, Pavel Butsykin wrote:
>>
>>
>> On 31.05.2019 19:06, Kirill Tkhai wrote:
>>> On 31.05.2019 12:23, Pavel Butsykin wrote:
>>>> There are still some number of response req types:
>>>> PCS_CS_WRITE_ZERO_RESP
>>>> PCS_CS_WRITE_HOLE_RESP
>>>> PCS_CS_REPLICATEX_RESP
>>>> PCS_CS_FIEMAP_RESP
>>>> PCS_CS_MAP_PROP_RESP
>>>>
>>>> It makes no sense to count statistics for such requests, so for them the stat
>>>> count will simply be skipped.
>>>>
>>>> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
>>>> ---
>>>>    fs/fuse/kio/pcs/fuse_stat.c | 7 ++++---
>>>>    1 file changed, 4 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/fs/fuse/kio/pcs/fuse_stat.c b/fs/fuse/kio/pcs/fuse_stat.c
>>>> index 06be3c75240c..31ff73d6c3dc 100644
>>>> --- a/fs/fuse/kio/pcs/fuse_stat.c
>>>> +++ b/fs/fuse/kio/pcs/fuse_stat.c
>>>> @@ -654,7 +654,6 @@ struct fuse_val_stat *req_stat_entry(struct pcs_fuse_io_stat *io, u32 type)
>>>>    		default:
>>>>    			break;
>>>>    	}
>>>> -	WARN_ON_ONCE(1);
>>>>    	return NULL;
>>>>    }
>>>>    
>>>> @@ -663,9 +662,11 @@ static void fuse_iostat_count(struct pcs_fuse_io_stat_sync *iostat,
>>>>    {
>>>>    	struct fuse_val_stat *se;
>>>>    
>>>> -	write_seqlock(&iostat->seqlock);
>>>>    	se = req_stat_entry(iostat->CURR(iostat), type);
>>>
>>> Shouldn't iostat->CURR(iostat) be obtained under seqlock?
>>>
>>
>> Yep, iostat->CURR(iostat) should be under seqlock, thanks for fixup!
> 
> I think it may be useful to add some lockdep check into CURR and other macros.

Yes, it makes sense, taking into account that it's not the most obvious
synchronization.

> Kirill
>
Pavel Butsykin June 3, 2019, 7:28 a.m.
On 03.06.2019 10:18, Pavel Butsykin wrote:
> 
> 
> On 03.06.2019 10:14, Kirill Tkhai wrote:
>> On 03.06.2019 09:41, Pavel Butsykin wrote:
>>>
>>>
>>> On 31.05.2019 19:06, Kirill Tkhai wrote:
>>>> On 31.05.2019 12:23, Pavel Butsykin wrote:
>>>>> There are still some number of response req types:
>>>>> PCS_CS_WRITE_ZERO_RESP
>>>>> PCS_CS_WRITE_HOLE_RESP
>>>>> PCS_CS_REPLICATEX_RESP
>>>>> PCS_CS_FIEMAP_RESP
>>>>> PCS_CS_MAP_PROP_RESP
>>>>>
>>>>> It makes no sense to count statistics for such requests, so for them the stat
>>>>> count will simply be skipped.
>>>>>
>>>>> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
>>>>> ---
>>>>>     fs/fuse/kio/pcs/fuse_stat.c | 7 ++++---
>>>>>     1 file changed, 4 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/fs/fuse/kio/pcs/fuse_stat.c b/fs/fuse/kio/pcs/fuse_stat.c
>>>>> index 06be3c75240c..31ff73d6c3dc 100644
>>>>> --- a/fs/fuse/kio/pcs/fuse_stat.c
>>>>> +++ b/fs/fuse/kio/pcs/fuse_stat.c
>>>>> @@ -654,7 +654,6 @@ struct fuse_val_stat *req_stat_entry(struct pcs_fuse_io_stat *io, u32 type)
>>>>>     		default:
>>>>>     			break;
>>>>>     	}
>>>>> -	WARN_ON_ONCE(1);
>>>>>     	return NULL;
>>>>>     }
>>>>>     
>>>>> @@ -663,9 +662,11 @@ static void fuse_iostat_count(struct pcs_fuse_io_stat_sync *iostat,
>>>>>     {
>>>>>     	struct fuse_val_stat *se;
>>>>>     
>>>>> -	write_seqlock(&iostat->seqlock);
>>>>>     	se = req_stat_entry(iostat->CURR(iostat), type);
>>>>
>>>> Shouldn't iostat->CURR(iostat) be obtained under seqlock?
>>>>
>>>
>>> Yep, iostat->CURR(iostat) should be under seqlock, thanks for fixup!
>>
>> I think it may be useful to add some lockdep check into CURR and other macros.
> 
> Yes, it makes sense, taking into account that it's not the most obvious
> synchronization.
> 

Unfortunately, the use of this macro does not always mean taking seqlock,
examples:
pcs_fuse_fstat_alloc(), pcs_fuse_fstat_free(), pcs_fuse_io_stat_alloc().

>> Kirill
>>
> 
> _______________________________________________
> Devel mailing list
> Devel@openvz.org
> https://lists.openvz.org/mailman/listinfo/devel
>