Description of problem: When we run i.e. lvconvert command and in parallel 'lvs' - lvs may read & use ANY data stored on this 'during' conversion progress and then later after it gains READ lock - it uses this basically historical garbage to access DM nodes and read i.e. status information from them. This optimization introduced with commit: aee27dc7bad5734012885fe9f174def0a3f26771 needs to be reverted and must not be used for standard running command. If we do want to support fast (but occasionally totally wrong) 'lock-less' disk scanning commands - let's use this with some specific option separately. Version-Release number of selected component (if applicable): 2.03.07 How reproducible: Steps to Reproduce: 1. i.e. in loop in 1st. shell lvconvert -m+1; lvconvert -m0 2. in 2nd. shell in loop 'lvs' 3. Actual results: Expected results: Additional info: can be temporarily 'quickly' eliminated with something like this: diff --git a/tools/lvmcmdline.c b/tools/lvmcmdline.c index 860e6de3a..ab32566b0 100644 --- a/tools/lvmcmdline.c +++ b/tools/lvmcmdline.c @@ -2386,7 +2386,7 @@ static int _get_current_settings(struct cmd_context *cmd) cmd->lockd_vg_default_sh = 1; if (cmd->cname->flags & CAN_USE_ONE_SCAN) - cmd->can_use_one_scan = 1; + cmd->can_use_one_scan = 0; cmd->include_exported_vgs = (cmd->cname->flags & ALLOW_EXPORTED) ? 1 : 0;
pushed fix disabling the optimzation to master: https://sourceware.org/git/?p=lvm2.git;a=commit;h=7474440d3b540d20eb4f997efeb31b881cc6ac8e We could bring back the optimization if we could detect when another command has updated the VG or taken the exclusive file lock (which would indicate the VG has likely been modified.) A traditional way to do that would be updating the lock file timestamp, or keeping the seqno (counter) in the lock file. The lvs command would then save the timestamp/seqno before reading, check the timestamp/seqno after taking the lock, and only reread when it's changed. This would also require keeping lock files around between commands instead of unlinking and recreating from each command.
I believe if user wants to see 'whatever fits just quickly' - doing 'lock less' path (so avoiding completely taking read-lock and just using whatever has been found during 'scan' - can be an option. All other solutions which are 'lock' based are most likely going to end up with more complex logic once you start to consider 'parallel' work of many commands running concurrently and working on multiple VGs. Also 'vg/lvdisplay, vgs' is in same rank as 'lvs'. And commands like 'pvs/pvdisplay' also exposes some info for fields where parsing of metadata is required - in this case again we need to access only metadata which are result of finished command processing - otherwise we may process 'some' metadata which are result of i.e. 'temporary' metadata modification (thought it's hard to quantize impact on correctness in this case) ATM basically I'd use 'optimized' access only when user asked for lock less results - in this case race errors are expected.
Another way around - when user specifies 'vgname' - we can take the lock before doing 1st. scan - and keep at across whole command. So 'lvs vgname' would be faster/optimal compared with just plain 'lvs'. (some sort of common speedup across majority of commands working with a single vg)
Yes, that's a good idea. It's a different optimization (prelocking the vgname when it's provided by the user) and is something I've had planned for some time. There's a TODO comment in label.c about adding it. I've implemented the lock file timestamps as mentioned above to resolve the problem with the original optimization. It's working nicely for me, I'll push to a branch to look at tomorrow.
I'd think that playing with file timestamps would be probably more expensive, than just rereading PV header sector/4K block and comparing it's not changed. Also timestamps would only work, if all the commands on system would use them and we know what are container users doing... So adding to io manager 'revalidation' based on checking few important bits in header blocks - and if they match - keep the rest of cached content - might look reasonable. Although I'd say cache of parsed metadata tree is more needed.