We can't keep adding IBITs without defining some ordering or hierarchy for when we use them.
That supposedly means that the list below shows a deadlock or something of that kind.
However, nothing prevents EX from being granted. After the EX lock is granted and setxattr succeeds, the client will release the PW lock and the PR lock request can succeed.
Try it and see. Or you can just do something like:
MOUNT_2=y llmount.sh cd /mnt/lustre while true; do setfattr -n user.ATTR -v LULZ f0; done & cd /mnt/lustre2 while true; do getfattr -n user.ATTR f0; done &
Soon both handlers will hang and you'll have a resource that looks like:
Resource: [0x200000400:0x6f:0x0].0 (ffff880194bb20c0) refcount = 6 Granted locks (in reverse order): ### ### ns: mdt-lustre-MDT0000_UUID lock: ffff880187b3e2c0/0x20a0719b52891d17 lrc: 3/0,0 mode: PW/PW res: [0x200000400:0x6f:0x0].0 bits 0x20 rrc: 6 type: IBT flags: 0x60200000000020 nid: 0@lo remote: 0x20a0719b52891d09 expref: 6 pid: 20067 timeout: 4296110059 lvb_type: 0 ### ### ns: mdt-lustre-MDT0000_UUID lock: ffff88018fbe3d40/0x20a0719b52891bdc lrc: 2/0,0 mode: CR/CR res: [0x200000400:0x6f:0x0].0 bits 0x8 rrc: 6 type: IBT flags: 0x40000000000000 nid: 0@lo remote: 0x20a0719b52891bd5 expref: 8 pid: 17987 timeout: 0 lvb_type: 3 Waiting locks: ### ### ns: mdt-lustre-MDT0000_UUID lock: ffff880194bfd3c0/0x20a0719b52891d2c lrc: 3/1,0 mode: --/PR res: [0x200000400:0x6f:0x0].0 bits 0x20 rrc: 6 type: IBT flags: 0x40210000000000 nid: local remote: 0x0 expref: -99 pid: 17987 timeout: 0 lvb_type: 0 ### ### ns: mdt-lustre-MDT0000_UUID lock: ffff88018f920180/0x20a0719b52891d33 lrc: 3/0,1 mode: --/EX res: [0x200000400:0x6f:0x0].0 bits 0x2 rrc: 6 type: IBT flags: 0x40210000000000 nid: local remote: 0x0 expref: -99 pid: 20067 timeout: 0 lvb_type: 0
Indeed, the EX lock was added to the waiting list to wait for cancellation of some other lock and this is after that lock was canceled. But the EX lock will not be granted since the head of the waiting list is blocked. Please see http://review.whamcloud.com/#/c/7148/ for some discussion.
Right. Thanks for the detailed explanation, John.
By the way, how easily can you reproduce the crash?
It reproduces within seconds of starting the two loops.
How could you see the deadlock then? 
Never mind, I was able to reproduce it on my hardware.
http://review.whamcloud.com/#/c/7191/1 for the crash part
Using http://review.whamcloud.com/#/c/7148/ and http://review.whamcloud.com/#/c/7191/ seems to make racer.sh pass (no traces of eviction or crashes for a few runs).
John, is there a chance that 7148 will be landed or should locking be redesigned anyway?
Hi Andrew, I was just updating the ticket when I saw your comment.
> It reproduces within seconds of starting the two loops.
Sorry, I misread your question. The hang reproduces within seconds of starting the two loops. For the crash use http://review.whamcloud.com/#/c/5936/ and run racer. Four times out of five it will hang the other time it will crash. I pulled your patch for the crash and it seems OK.
> John, is there a chance that 7148 will be landed or should locking be redesigned anyway?
Not sure. I'm working to get reviewers on it.
I'm still concerned that 7148 + 7191 are not enough. Consider the following case:
# MOUNT_2=y llmount.sh # cd /mnt/lustre # while true; do touch f0; rm f0; done & # cd /mnt/lustre2 # while true; do setfattr -n user.ATTR -v LULZ f0; done &
Eventually the setattr gets a PW XATTR lock, the unlink enqueues a blocked EX FULL lock, and the setattr enqueues a blocked EX UPDATE lock. Sorry I didn't mention it earlier, I only thought of it this morning.
The hang reproduces within seconds of starting the two loops.
OK, I see.
I pulled your patch for the crash and it seems OK.
Great! Thanks a lot for the feedback.
Eventually the setattr gets a PW XATTR lock, the unlink enqueues a blocked EX FULL lock, and the setattr enqueues a blocked EX UPDATE lock. Sorry I didn't mention it earlier, I only thought of it this morning.
Hm, right. It looks like nested LDLM locking is often a headache. 
We've discussed the issue with our team, and we think the following might be a solution:
1. UPDATE locking should probably be removed from the MDT setxattr path, because it does not seem to be protecting any piece of data [needs a bit more investigation]
2. POSIX ACL ACCESS data should be filtered out from the xattr cache.
3. getxattr and setxattr on POSIX ACLs should use the old (non-xattrcache) code path
4. listxattr should use the xattr cache and also additionally list XATTR_NAME_ACL_ACCESS ("system.posix_acl_access") if it finds that lli_posix_acl is initialized.
This splits xattr cache and ACL cache and so splits XATTR and LOOKUP locking.
Panda, I think your proposal to separate ACL xattrs from xattr cache is a reasonable one.
The other thing that would reduce lock contention would be to only grant the PR lock if the lock is under contention, as is done in mdt_getattr_name_lock() for the UPDATE lock. This allows clients to cache readdir data and negative dentries for static directories, but does not cause lock ping-pong when the directory is being modified:
/* If the file has not been changed for some time, we * return not only a LOOKUP lock, but also an UPDATE * lock and this might save us RPC on later STAT. For * directories, it also let negative dentry starts * working for this dir. */ if (ma->ma_valid & MA_INODE && ma->ma_attr.la_valid & LA_CTIME && info->mti_mdt->mdt_namespace->ns_ctime_age_limit + ma->ma_attr.la_ctime < cfs_time_current_sec()) child_bits |= MDS_INODELOCK_UPDATE;
Doing something similar for the xattr cache would allow e.g. Samba servers to have a writethrough cache, normal clients to have a read cache for most files, and no cache for files that are being modified frequently (which is no worse than we have today).
NB - if you change code near here, please s/dentry starts/dentry cachestart/ in the above comment.
With Change, 7208 landing to Master, can this ticket be closed or reduced from blocker?
Both patches landed to master.