[LU-17022] Clean up flags and locking in struct obd_device

• Type: Improvement
• Resolution: Unresolved
• Priority: Minor
• Severity: 3
• Assignee: Ronnie Sahlberg
• Reporter: Neil Brown

Description

struct obd_device contains a number of flags using a single-bit bit-field.  These require a spinlock to ensure atomic updates as it documented in a comment.  However many places update these flags without a lock. Conversely sometime the lock is taken for only reading the flag, which provides no value.

The common practice in Linux is to use set_bit() etc which is atomic and requires no locking.  The flags should be converted.  Where a flag is both checked and set within a spinlock region, test_and_set_bit() or test_and_clear_bit() can be used.

The lock in question -> obd_dev_lock, is documented only as protecting these flag bit, but it actually protects a number of lists and other things.  This should be documented.

One of the fields it protects is  obd_conn_inprogress which would work better as an atomic_t, and which is waited for with a 'yield()' loop which is not good practice.  This should use wait_var_event etc.

The lock is also held across code that does not need locking, and this should be removed.

The name of the lock is easily confused with the global lock "obd_dev_lock".  One of these should be renamed.  Maybe obd_devs_lock for the global.

struct obd_export has similar issues deserving attention.

 

I propose to submit a number of patches to achieve all this with all but the last marked "trivial" so the heavy testing is only run on the complete set.

Issue Links

• is related to: LU-18231 Change all obd_device flags to use atomic set/create/test_bit operations