[PATCH V2 1/2] libblkid: Don't check BLKID_PROBE_INTERVAL in blkid_verify

[PATCH V2 1/2] libblkid: Don't check BLKID_PROBE_INTERVAL in blkid_verify

[Nikolay Borisov]

That constant is set to 200 seconds and is already check in probe_all().        
It essentially controls how often blkid_probe_all can do a full cache           
revalidation. Since blkid_verify is called from within probe_all() iff at least 
BLKID_PROBE_INTERVAL seconds have elapsed it makes no sense to check this value 
in blkid_verify. 

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
V2: 
 * Fixed stray { 
 libblkid/src/verify.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/libblkid/src/verify.c b/libblkid/src/verify.c
index 7f44f5497c8e..e630ab1a38c5 100644
--- a/libblkid/src/verify.c
+++ b/libblkid/src/verify.c
@@ -96,8 +96,7 @@ blkid_dev blkid_verify(blkid_cache cache, blkid_dev dev)
 	    st.st_mtime <= dev->bid_time &&
 #endif
 	    (diff < BLKID_PROBE_MIN ||
-		(dev->bid_flags & BLKID_BID_FL_VERIFIED &&
-		 diff < BLKID_PROBE_INTERVAL)))
+		dev->bid_flags & BLKID_BID_FL_VERIFIED))
 		return dev;
 
 #ifndef HAVE_STRUCT_STAT_ST_MTIM_TV_NSEC
-- 
2.7.4

[PATCH V2 2/2] libblkid: Set BLKID_BID_FL_VERIFIED in case revalidation is not needed

[Nikolay Borisov]

If blkid_verify deems that device revalidation is not needed since BLKID_PROBE_MIN
seconds haven't elapsed since the last revalidation of the device it returns    
a blkid_dev. However, if this device has been read from the cache file on disk then 
it wont' have BLKID_BID_FL_VERIFIED bit set. This in turn could lead to a later 
call to blkid_dev_get free this device in case its part of a multi device       
configuration. This is particularly relevant to btrfs raid configurations.      
                                                                                
Namely this  was exhibited by btrfs-progs which use blkid for device enumeration.
In case of a multi-device filesystem (i.e raid10) running xfstest btrfs/011     
would fail sporadically since cached devices read from cache were not revalidated
due to being recently validated (according to timestamp in the cache file) at   
the same time their in-memory blkid_dev structures didn't have BLKID_BID_FL_VERIFIED
set. This lead to their untimely removal from the cache from blkid_get_dev.     
                                                                                
Fix this by setting the BLKID_BID_FL_VERIFIED when returning from blkid_verify  
with a valid device irrespective of whether full revalidation was performed or  
the device is deemed valid due to being recently validated.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
V2:
 * Adjust context based on previous patch's fix. 

 libblkid/src/verify.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/libblkid/src/verify.c b/libblkid/src/verify.c
index e630ab1a38c5..58b347751e73 100644
--- a/libblkid/src/verify.c
+++ b/libblkid/src/verify.c
@@ -96,8 +96,10 @@ blkid_dev blkid_verify(blkid_cache cache, blkid_dev dev)
 	    st.st_mtime <= dev->bid_time &&
 #endif
 	    (diff < BLKID_PROBE_MIN ||
-		dev->bid_flags & BLKID_BID_FL_VERIFIED))
+		dev->bid_flags & BLKID_BID_FL_VERIFIED)) {
+		dev->bid_flags |= BLKID_BID_FL_VERIFIED;
 		return dev;
+	}
 
 #ifndef HAVE_STRUCT_STAT_ST_MTIM_TV_NSEC
 	DBG(PROBE, ul_debug("need to revalidate %s (cache time %lu, stat time %lu,\t"
-- 
2.7.4

Re: [PATCH V2 2/2] libblkid: Set BLKID_BID_FL_VERIFIED in case revalidation is not needed

[Nikolay Borisov]

On 13.05.19 г. 15:44 ч., Nikolay Borisov wrote:
> If blkid_verify deems that device revalidation is not needed since BLKID_PROBE_MIN
> seconds haven't elapsed since the last revalidation of the device it returns    
> a blkid_dev. However, if this device has been read from the cache file on disk then 
> it wont' have BLKID_BID_FL_VERIFIED bit set. This in turn could lead to a later 
> call to blkid_dev_get free this device in case its part of a multi device       
> configuration. This is particularly relevant to btrfs raid configurations.      
>                                                                                 
> Namely this  was exhibited by btrfs-progs which use blkid for device enumeration.
> In case of a multi-device filesystem (i.e raid10) running xfstest btrfs/011     
> would fail sporadically since cached devices read from cache were not revalidated
> due to being recently validated (according to timestamp in the cache file) at   
> the same time their in-memory blkid_dev structures didn't have BLKID_BID_FL_VERIFIED
> set. This lead to their untimely removal from the cache from blkid_get_dev.     
>                                                                                 
> Fix this by setting the BLKID_BID_FL_VERIFIED when returning from blkid_verify  
> with a valid device irrespective of whether full revalidation was performed or  
> the device is deemed valid due to being recently validated.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
> V2:
>  * Adjust context based on previous patch's fix. 
> 
>  libblkid/src/verify.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/libblkid/src/verify.c b/libblkid/src/verify.c
> index e630ab1a38c5..58b347751e73 100644
> --- a/libblkid/src/verify.c
> +++ b/libblkid/src/verify.c
> @@ -96,8 +96,10 @@ blkid_dev blkid_verify(blkid_cache cache, blkid_dev dev)
>  	    st.st_mtime <= dev->bid_time &&
>  #endif
>  	    (diff < BLKID_PROBE_MIN ||
> -		dev->bid_flags & BLKID_BID_FL_VERIFIED))
> +		dev->bid_flags & BLKID_BID_FL_VERIFIED)) {
> +		dev->bid_flags |= BLKID_BID_FL_VERIFIED;

Actually I think this patch is wrong because the check for
dev->bid_flags & BLKID_BID_FL_VERIFIED should also be removed. E.g.
BID_FL_VERIFIED should be set but check should be only diff <
BLKID_PROBE_MIN. Otherwise if once this flag is set either due to proper
revalidation or because not enough time has elapsed then it will never
be reset, because the device will never be freed.

>  		return dev;
> +	}
>  
>  #ifndef HAVE_STRUCT_STAT_ST_MTIM_TV_NSEC
>  	DBG(PROBE, ul_debug("need to revalidate %s (cache time %lu, stat time %lu,\t"
> 

Re: [PATCH V2 2/2] libblkid: Set BLKID_BID_FL_VERIFIED in case revalidation is not needed

[Karel Zak]

On Mon, May 13, 2019 at 05:21:33PM +0300, Nikolay Borisov wrote:
> >  	    (diff < BLKID_PROBE_MIN ||
> > -		dev->bid_flags & BLKID_BID_FL_VERIFIED))
> > +		dev->bid_flags & BLKID_BID_FL_VERIFIED)) {
> > +		dev->bid_flags |= BLKID_BID_FL_VERIFIED;
> 
> Actually I think this patch is wrong because the check for
> dev->bid_flags & BLKID_BID_FL_VERIFIED should also be removed. E.g.
> BID_FL_VERIFIED should be set but check should be only diff <
> BLKID_PROBE_MIN. Otherwise if once this flag is set either due to proper
> revalidation or because not enough time has elapsed then it will never
> be reset, because the device will never be freed.

Yes, we have only one time limit there (after your first patch) than
we don't need to differentiate between in-memory and from-cache. It
should be enough to check for the time and set the flag.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH V2 2/2] libblkid: Set BLKID_BID_FL_VERIFIED in case revalidation is not needed

[Karel Zak]

On Mon, May 13, 2019 at 05:21:33PM +0300, Nikolay Borisov wrote:
> >  	    (diff < BLKID_PROBE_MIN ||
> > -		dev->bid_flags & BLKID_BID_FL_VERIFIED))
> > +		dev->bid_flags & BLKID_BID_FL_VERIFIED)) {
> > +		dev->bid_flags |= BLKID_BID_FL_VERIFIED;
> 
> Actually I think this patch is wrong because the check for
> dev->bid_flags & BLKID_BID_FL_VERIFIED should also be removed. E.g.

Merged (both patches) and BLKID_BID_FL_VERIFIED removed. Thanks!

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com