Fixes for relase
cover Fixes for relase
Contains minimal consistent patchset for a release.
1st patch might need very simply 'hack' to make it work properly. But currently I fail to see how :(
Others are fixes releated to multiple stacked operations.
As we have had still left fs_unlock with every lv_resume() these were not really a big problem - but once this is removed the lv_info() check showed as the best place for checking udev sychronization and making sure there are no operations in progress. (This is very quick one the cookie is 0)
Zdenek Kabelac (5):
Fix clustered and no-lock locking with HOTFIX
Move add_dev_node to DM_DEVICE_RESUME
Fix operation node stacking
lv_info handle udev_sync
Remove open_count read from some lv_info calls
daemons/clvmd/clvmd-command.c | 5 ++- daemons/clvmd/clvmd.c | 3 ++ daemons/clvmd/lvm-functions.c | 9 ++++- daemons/clvmd/lvm-functions.h | 2 +- lib/activate/activate.c | 15 +++++++- lib/activate/fs.c | 66 ++++++++++++++++++++++++++++++++++++-- lib/activate/fs.h | 1 + lib/locking/cluster_locking.c | 6 ++-- lib/locking/locking.c | 7 ++-- lib/locking/locking.h | 22 +++++++++++- libdm/ioctl/libdm-iface.c | 8 ++-- libdm/libdm-common.c | 71 ++++++++++++++++++++++++++++++++++++----- liblvm/lvm_lv.c | 4 +- tools/lvconvert.c | 2 +- tools/lvscan.c | 2 +- 15 files changed, 190 insertions(+), 33 deletions(-)
Signature · 1 line
1.7.3.5
PATCH 1/5 Fix clustered and no-lock locking with HOTFIX
Patch is fixing clustered locking - but currently cluster wide message passing does not work with sync_dev_names().
To avoid any udev synchronizaiton problems with leaving unrelased udev cookies, revert to the old slow behavior - and check for udev cookie with after every activate and deactivate operation inside do_lock_vol().
Once the new message will pass around the cluster - these extra fs_unlock() shall be removed.
The problem I have now:
Cluster does not pass any message around cluster related to unlock_vg. So the initial idea to use this point for fs_unlock() works only on the local/single node.
For the real multinode cluster we need to pass extra message around
- but currently I've not managed to pass this new message around.
We cannot reuse any other message like DROP_CACHE or VG_BACKUP as this are send when MDA is modifed - however activation does not modify anything - yet it needs to flush udev cookie.
sync_local_dev_names() - serves for local cluster synchronization sync_dev_names() - is supposed to be send to all nodes in cluster to sychronizes its dev nodes. This should happen when vg is unlocked.
For this - macro 'unlock_vg()' is modifed to call sync_dev_names for clustered locking before it actually releases VG lock - it's called before lock_vol code to avoid recusion.
Mornfal expressed concerns about possibility to unlock VG by plain call of lock_vol() with right parameters - this would miss the sync call - however I'm currently not seeing better way, how to achieve message passing around - unless we find the way to to pass send this message within clvmd unlock_vg processing - the code is currently not very clear in this area.
So once we figure out, how to pass such parameter - we may add implicit sync_dev_names() into unlock clvmd code and no-locking code.
For filesystem and external locking implicit fs_unlock() is called with lock_vol as there is no recursive rick in this case.
For sync names - new LCK is defined:LCK_VG_SYNC - which should differ from LCK_VG_BACKUP by LCK_HOLD bit. But somehow I fail to see the reason why this message is not send around the cluster and only local node is processing this message.
Difference between local & global sync is LCL_LOCAL and usage of VG_SYNC_NAMES for local message.
As a potential solution could be - to overload DROP_CACHE and with special paramater use this for as VG_SYNC_NAMES
So the cache would be dropped in other cases.
But my head already spins quite a lot around....
So any help would be appreciated.
--- daemons/clvmd/clvmd-command.c | 5 ++++- daemons/clvmd/clvmd.c | 3 +++ daemons/clvmd/lvm-functions.c | 9 +++++++-- daemons/clvmd/lvm-functions.h | 2 +- lib/locking/cluster_locking.c | 6 +++--- lib/locking/locking.c | 7 ++++--- lib/locking/locking.h | 22 ++++++++++++++++++++-- 7 files changed, 42 insertions(+), 12 deletions(-)
daemons/clvmd/clvmd-command.c +4 -1
@@ -139,7 +139,10 @@ int do_command(struct local_client *client, struct clvm_header *msg, int msglen,break ;case CLVMD_CMD_SYNC_NAMES : -lvm_do_fs_unlock (); + lock_cmd = args[0 ]; + lock_flags = args[1 ]; + lockname = &args[2 ]; +lvm_do_fs_unlock (lock_cmd, lock_flags, lockname, client ?1 :0 );break ;case CLVMD_CMD_SET_DEBUG :
daemons/clvmd/clvmd.c +3 -0
@@ -296,6 +296,9 @@ static const char *decode_cmd(unsigned char cmdl)case CLVMD_CMD_RESTART : command ="RESTART" ;break ; +case CLVMD_CMD_SYNC_NAMES : + command ="SYNC_NAMES" ; +break ;default : command ="unknown" ;break ;
daemons/clvmd/lvm-functions.c +7 -2
@@ -507,6 +507,7 @@ int do_lock_lv(unsigned char command, unsigned char lock_flags, char *resource)switch (command &LCK_MASK ) {case LCK_LV_EXCLUSIVE : status =do_activate_lv (resource, lock_flags,LCK_EXCL ); +fs_unlock ();break ;case LCK_LV_SUSPEND :
@@ -520,10 +521,12 @@ int do_lock_lv(unsigned char command, unsigned char lock_flags, char *resource)case LCK_LV_ACTIVATE : status =do_activate_lv (resource, lock_flags,LCK_READ ); +fs_unlock ();break ;case LCK_LV_DEACTIVATE : status =do_deactivate_lv (resource, lock_flags); +fs_unlock ();break ;default :
@@ -894,11 +897,13 @@ struct dm_hash_node *get_next_excl_lock(struct dm_hash_node *v, char **name)return v; } -void lvm_do_fs_unlock (void ) +void lvm_do_fs_unlock (int lock_cmd,int lock_flags,char *lockname,int is_client) {pthread_mutex_lock (&lvm_lock); -DEBUGLOG ("Syncing device names\n" ); +DEBUGLOG ("Syncing device names cmd=%d flags=%d name=%s client=%d\n" , + lock_cmd, lock_flags, lockname, is_client);fs_unlock (); +pthread_mutex_unlock (&lvm_lock); }
daemons/clvmd/lvm-functions.h +1 -1
@@ -36,6 +36,6 @@ extern char *get_last_lvm_error(void);extern void do_lock_vg (unsigned char command,unsigned char lock_flags,char *resource);extern struct dm_hash_node *get_next_excl_lock (struct dm_hash_node *v,char **name); -void lvm_do_fs_unlock (void ); +void lvm_do_fs_unlock (int lock_cmd,int lock_flags,char *lockname,int is_client);#endif
lib/locking/cluster_locking.c +3 -3
@@ -401,10 +401,10 @@ int lock_resource(struct cmd_context *cmd, const char *resource, uint32_t flags)switch (flags &LCK_SCOPE_MASK ) {case LCK_VG : -if (!strcmp (resource,VG_SYNC_NAMES )) { -log_very_verbose ("Requesting sync names." ); +if ((flags & ~LCK_LOCAL ) ==LCK_VG_SYNC ) { +log_very_verbose ("Requesting sync names for %s." , resource);return _lock_for_cluster (cmd,CLVMD_CMD_SYNC_NAMES , - flags & ~LCK_HOLD , resource); + flags, resource); }if (flags ==LCK_VG_BACKUP ) { log_very_verbose("Requesting backup of VG metadata for %s",
lib/locking/locking.c +4 -3
@@ -443,9 +443,10 @@ int lock_vol(struct cmd_context *cmd, const char *vol, uint32_t flags)if (!check_lvm1_vg_inactive (cmd, vol)) return_0; -/* Before unlocking VG wait until devices are available. */ -if ((flags &LCK_TYPE_MASK ) ==LCK_UNLOCK ) -sync_local_dev_names (cmd); +/* For non clustered unlock - implicit fs_unlock() */ +if ((lck_type ==LCK_UNLOCK ) && !(flags &LCK_CACHE ) && +is_real_vg (vol) && !locking_is_clustered ()) +fs_unlock ();break ;case LCK_LV :/* All LV locks are non-blocking. */
lib/locking/locking.h +20 -2
@@ -126,6 +126,7 @@ int check_lvm1_vg_inactive(struct cmd_context *cmd, const char *vgname);#define LCK_VG_REVERT (LCK_VG | LCK_READ | LCK_CACHE | LCK_HOLD)#define LCK_VG_BACKUP (LCK_VG | LCK_CACHE) +#define LCK_VG_SYNC (LCK_VG | LCK_CACHE | LCK_HOLD)#define LCK_LV_EXCLUSIVE (LCK_LV | LCK_EXCL)#define LCK_LV_SUSPEND (LCK_LV | LCK_WRITE)
@@ -143,7 +144,20 @@ int check_lvm1_vg_inactive(struct cmd_context *cmd, const char *vgname); lock_vol(cmd, (lv)->lvid.s, flags | LCK_LV_CLUSTERED(lv)) : \ 0) -#define unlock_vg (cmd, vol) lock_vol(cmd, vol, LCK_VG_UNLOCK) +/* +* For clustered locking (cluster, no_lock) explicit call +* of cluster wide sync_dev_names() is made before unlocking vg. +* For non-clustered locking (filesystem, external) +* use of fs_unlock() is implicit in lock_vol(). +*/ +#define unlock_vg (cmd, vol) \ + do { \ + if (is_real_vg(vol) && \ + locking_is_clustered()) \ + sync_dev_names(cmd, vol); \ + lock_vol(cmd, vol, LCK_VG_UNLOCK); \ + } while (0) +#define unlock_and_free_vg (cmd, vg, vol) \ do { \ unlock_vg(cmd, vol); \
@@ -170,8 +184,12 @@ int check_lvm1_vg_inactive(struct cmd_context *cmd, const char *vgname); lock_vol((vg)->cmd, (vg)->name, LCK_VG_REVERT)#define remote_backup_metadata (vg) \ lock_vol((vg)->cmd, (vg)->name, LCK_VG_BACKUP) +/* Synchronize local device names on cluster node */ #define sync_local_dev_names (cmd) \ - lock_vol(cmd, VG_SYNC_NAMES, LCK_NONE | LCK_CACHE | LCK_LOCAL) + lock_vol(cmd, VG_SYNC_NAMES, LCK_VG_SYNC | LCK_LOCAL) +/* Synchronize global device names on all cluster nodes */ +#define sync_dev_names (cmd, vol) \ + lock_vol(cmd, vol, LCK_VG_SYNC)/* Process list of LVs */ int suspend_lvs (struct cmd_context *cmd,struct dm_list *lvs);
Signature · 1 line
1.7.3.5
PATCH 2/5 Move add_dev_node to DM_DEVICE_RESUME
In reply to Zdenek KabelacThis patch came out from discussion with Petr. We need valid 'check_udev' for stacked ops processing.
==
Current code:
dmsetup create xxx --notable -> create node in /dev/mapper/xxx
(check_udev has incorrect 0 value) and we are forcing node creation in /dev directory - while only udevd should have full control over the nodes available there.
(i.e. selinux policy might allo creation only in udev process)
==
New code creates /dev/mapper node with:
dmsetup resume xxx
As just at this moment udev is registering such device through a CHANGE event.
==
This patch is causing incompatibilities with some old hand written scripts which may relay on existance of device node even when such node has only empty table.
But the functionality is needed for following patch which prunes operation on the same node when they are all combined within one udev cookie transaction.
We cannot insert there 'ADD' operation which pretends it should not check for udev and mix them with udev operation - as we would again modify content of /dev dir.
Also this patch allows to further cleanup libdevmapper code from other quirks.
--- libdm/ioctl/libdm-iface.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-)
libdm/ioctl/libdm-iface.c +4 -4
@@ -2064,10 +2064,6 @@ repeat_ioctl:switch (dmt->type) {case DM_DEVICE_CREATE : -if (dmt->dev_name && *dmt->dev_name && !udev_only) -add_dev_node (dmt->dev_name,MAJOR (dmi->dev), -MINOR (dmi->dev), dmt->uid, dmt->gid, - dmt->mode, check_udev);break ;case DM_DEVICE_REMOVE :/* FIXME Kernel needs to fill in dmi->name */
@@ -2083,6 +2079,10 @@ repeat_ioctl:break ;case DM_DEVICE_RESUME : +if (dmt->dev_name && *dmt->dev_name && !udev_only) +add_dev_node (dmt->dev_name,MAJOR (dmi->dev), +MINOR (dmi->dev), dmt->uid, dmt->gid, + dmt->mode, check_udev);/* FIXME Kernel needs to fill in dmi->name */ set_dev_node_read_ahead (dmt->dev_name, dmt->read_ahead, dmt->read_ahead_flags);
Signature · 1 line
1.7.3.5
PATCH 3/5 Fix operation node stacking
In reply to Zdenek KabelacWith ability to stack many operations in one udev transaction - in same cases we are adding and removing same device at the same time.
This leads to a problem of checking stacked operations:
i.e. remove /dev/node1 followed by create /dev/node1
If the node creation is handled with udev - there is a problem as stacked operation gives warning about existing node1 and will try to remove it - while next operation needs to recreate it.
Current code removes all previous stacked operation if the fs op is FS_DEL - patch adds similar behavior for FS_ADD - it will try to remove any 'delete' operation if udev is in use.
For FS_RENAME operation it seems to be more complex. But as we are always stacking FS_READ_AHEAD after FS_ADD operation - should be safe to remove all previous operation on the node when udev is running.
Code does same checking for stacking libdm and liblvm operations.
As a very simple optimization counters were added for each stacked ops type to avoid unneeded list scans if some operation does not exists in the list.
WARNING:
Patch checks variable 'check_udev' flag so the previous patch is mandatory
--- lib/activate/fs.c | 61 ++++++++++++++++++++++++++++++++++++++++-- libdm/libdm-common.c | 71 ++++++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 121 insertions(+), 11 deletions(-)
lib/activate/fs.c +58 -3
@@ -257,7 +257,8 @@ static int _rm_link(const char *dev_dir, const char *vg_name,typedef enum {FS_ADD ,FS_DEL , -FS_RENAME +FS_RENAME , +NUM_FS_OPS }fs_op_t ; static int _do_fs_op(fs_op_t type, const char *dev_dir, const char *vg_name,
@@ -283,12 +284,18 @@ static int _do_fs_op(fs_op_t type, const char *dev_dir, const char *vg_name,if (!_mk_link (dev_dir, vg_name, lv_name, dev, check_udev)) stack; +default :; }return 1 ; }static DM_LIST_INIT (_fs_ops); +/* +* Count number of stacked fs_op_t operations to allow to skip dm_list search. +* FIXME: handling of FS_RENAME +*/ +static int _c_fs_ops[NUM_FS_OPS ];struct fs_op_parms {struct dm_list list;
@@ -309,15 +316,63 @@ static void _store_str(char **pos, char **ptr, const char *str) *pos +=strlen (*ptr) +1 ; } +static int _other_fs_ops (fs_op_t type) +{ +int i; + +for (i =0 ; i <NUM_FS_OPS ; i++) +if (type != i && _c_fs_ops[i]) +return 1 ; +return 0 ; +} + +static void _del_fs_op (struct fs_op_parms *fsp) +{ + _c_fs_ops[fsp->type]--; +dm_list_del (&fsp->list); +dm_free (fsp); +} +static int _stack_fs_op (fs_op_t type,const char *dev_dir,const char *vg_name,const char *lv_name,const char *dev,const char *old_lv_name,int check_udev) { +struct dm_list *fsph, *fspht;struct fs_op_parms *fsp;size_t len =strlen (dev_dir) +strlen (vg_name) +strlen (lv_name) +strlen (dev) +strlen (old_lv_name) +5 ;char *pos; +if ((type ==FS_DEL ) &&_other_fs_ops (type)) + dm_list_iterate_safe(fsph, fspht, &_fs_ops) { + fsp = dm_list_item(fsph, struct fs_op_parms); +if (!strcmp (lv_name, fsp->lv_name) && + !strcmp (vg_name, fsp->vg_name)) { +_del_fs_op (fsp); +if (!_other_fs_ops (type)) +break ; + } + } + else if ((type == FS_ADD) && _c_fs_ops[FS_DEL] && check_udev && + dm_udev_get_sync_support() && dm_udev_get_checking()) + dm_list_iterate_safe(fsph, fspht, &_fs_ops) { + fsp = dm_list_item(fsph, struct fs_op_parms); +if ((fsp->type ==FS_DEL ) && + !strcmp (lv_name, fsp->lv_name) && + !strcmp (vg_name, fsp->vg_name)) { +_del_fs_op (fsp); +break ; + } + } + else if ((type == FS_RENAME) && check_udev && + dm_udev_get_sync_support() && dm_udev_get_checking()) + dm_list_iterate_safe(fsph, fspht, &_fs_ops) { + fsp = dm_list_item(fsph, struct fs_op_parms); +if (!strcmp (old_lv_name, fsp->lv_name) && + !strcmp (vg_name, fsp->vg_name)) +_del_fs_op (fsp); + } +if (!(fsp =dm_malloc (sizeof (*fsp) + len))) {log_error ("No space to stack fs operation" );return 0 ;
@@ -333,6 +388,7 @@ static int _stack_fs_op(fs_op_t type, const char *dev_dir, const char *vg_name,_store_str (&pos, &fsp->dev, dev);_store_str (&pos, &fsp->old_lv_name, old_lv_name); + _c_fs_ops[type]++;dm_list_add (&_fs_ops, &fsp->list);return 1 ;
@@ -347,8 +403,7 @@ static void _pop_fs_ops(void) fsp = dm_list_item(fsph, struct fs_op_parms);_do_fs_op (fsp->type, fsp->dev_dir, fsp->vg_name, fsp->lv_name, fsp->dev, fsp->old_lv_name, fsp->check_udev); -dm_list_del (&fsp->list); -dm_free (fsp); +_del_fs_op (fsp); } }
libdm/libdm-common.c +63 -8
119 lines not on this page · raw message
Signature · 1 line
1.7.3.5
PATCH 4/5 lv_info handle udev_sync
In reply to Zdenek KabelacIn case open_count is requested via lv_info - check if there are any udev operations in-progress - and wait for them before checking for lv_info
Remove fs_unlock after _lv_resume - as it's now handled via lv_info query.
Note: some those lv_info() which do not need open_count, needs to pass 0 for 'with_open_count' parameter.
NoteII: locking_is_clustered() is needed to detect in which context lv_info() is called as we cannot use sync_local_dev_names() when locking is not defined.
--- lib/activate/activate.c | 13 ++++++++++++- lib/activate/fs.c | 5 +++++ lib/activate/fs.h | 1 + 3 files changed, 18 insertions(+), 1 deletions(-)
lib/activate/activate.c +12 -1
@@ -460,6 +460,18 @@ int lv_info(struct cmd_context *cmd, const struct logical_volume *lv, unsigned oif (!activation ())return 0 ; +/* +* If open_count info is requested and we have to be sure our own udev +* transactions are finished +* For non-clustered locking type we are only interested for non-delete operation +* in progress - as only those could lead to opened files +*/ +if (with_open_count) { +if (locking_is_clustered ()) +sync_local_dev_names (cmd);/* Wait to have udev in sync */ +else if (fs_has_non_delete_ops ()) +fs_unlock ();/* For non clustered - wait if there are non-delete ops */ + }if (!dev_manager_info (lv->vg->cmd->mem, lv, origin_only ?"real" :NULL , with_open_count, with_read_ahead, &dminfo, &info->read_ahead))
@@ -1112,7 +1124,6 @@ static int _lv_resume(struct cmd_context *cmd, const char *lvid_s, goto_out;memlock_dec (cmd); -fs_unlock ();if (!monitor_dev_for_events (cmd, lv, origin_only,1 )) stack;
lib/activate/fs.c +5 -0
@@ -477,3 +477,8 @@ void fs_set_cookie(uint32_t cookie) { _fs_cookie = cookie; } + +int fs_has_non_delete_ops (void ) +{ +return _other_fs_ops (FS_DEL ); +}
lib/activate/fs.h +1 -0
@@ -32,5 +32,6 @@ int fs_rename_lv(struct logical_volume *lv, const char *dev,/* void fs_unlock(void); moved to activate.h */ uint32_t fs_get_cookie (void );void fs_set_cookie (uint32_t cookie); +int fs_has_non_delete_ops (void );#endif
Signature · 1 line
1.7.3.5
PATCH 5/5 Remove open_count read from some lv_info calls
In reply to Zdenek KabelacPLEASE CHECK ME!!!
It's not completely clear what could be the side effectlv_infoi() in these places does not read 'info.open_count' value - so it should not need to set this value - unless I'm missing some side-effect??
This change is needed to minimalize the amount of fs_unlock() calls.
As we need to make sure no udev operation is in progress when we check for open_count.
--- lib/activate/activate.c | 2 +- liblvm/lvm_lv.c | 4 ++-- tools/lvconvert.c | 2 +- tools/lvscan.c | 2 +- 4 files changed, 5 insertions(+), 5 deletions(-)
lib/activate/activate.c +1 -1
@@ -1218,7 +1218,7 @@ int lv_deactivate(struct cmd_context *cmd, const char *lvid_s) r =_lv_deactivate (lv);memlock_dec (cmd); -if (!lv_info (cmd, lv,0 , &info,1 ,0 ) || info.exists) +if (!lv_info (cmd, lv,0 , &info,0 ,0 ) || info.exists) r =0 ; out:if (lv) {
liblvm/lvm_lv.c +2 -2
@@ -62,7 +62,7 @@ struct lvm_property_value lvm_lvseg_get_property(const lvseg_t lvseg,uint64_t lvm_lv_is_active (const lv_t lv) {struct lvinfo info; -if (lv_info (lv->vg->cmd, lv,0 , &info,1 ,0 ) && +if (lv_info (lv->vg->cmd, lv,0 , &info,0 ,0 ) && info.exists && info.live_table)return 1 ;return 0 ;
@@ -71,7 +71,7 @@ uint64_t lvm_lv_is_active(const lv_t lv)uint64_t lvm_lv_is_suspended (const lv_t lv) {struct lvinfo info; -if (lv_info (lv->vg->cmd, lv,0 , &info,1 ,0 ) && +if (lv_info (lv->vg->cmd, lv,0 , &info,0 ,0 ) && info.exists && info.suspended)return 1 ;return 0 ;
tools/lvconvert.c +1 -1
@@ -1637,7 +1637,7 @@ static int poll_logical_volume(struct cmd_context *cmd, struct logical_volume *l {struct lvinfo info; -if (!lv_info (cmd, lv,0 , &info,1 ,0 ) || !info.exists) { +if (!lv_info (cmd, lv,0 , &info,0 ,0 ) || !info.exists) {log_print ("Conversion starts after activation." );return ECMD_PROCESSED ; }
tools/lvscan.c +1 -1
@@ -28,7 +28,7 @@ static int lvscan_single(struct cmd_context *cmd, struct logical_volume *lv,if (!arg_count (cmd, all_ARG) && !lv_is_visible (lv))return ECMD_PROCESSED ; - inkernel =lv_info (cmd, lv,0 , &info,1 ,0 ) && info.exists; + inkernel =lv_info (cmd, lv,0 , &info,0 ,0 ) && info.exists;if (lv_is_origin (lv)) {dm_list_iterate_items_gen (snap_seg, &lv->snapshot_segs, origin_list) {
Signature · 1 line
1.7.3.5