what kernel version do you use?
this is actually not the locking broken but rather credits calculation (in optimization part)..
ofd_write_attrset(pid 13105) do declaration for 0 -> 0, then another thread(pid 13086) changed uid, and then ofd_write_attrset do ofd_read_lock, setattr.
3.10.0-957.1.3957.1.3.x3.4.160.x86_64
th = ofd_trans_create(env, ofd);
if (IS_ERR(th))
GOTO(out, rc = PTR_ERR(th));
if (la->la_valid) {
rc = dt_declare_attr_set(env, dt_obj, la, th);
if (rc)
GOTO(out_tx, rc);
}
rc = dt_declare_xattr_set(env, dt_obj, &info->fti_buf,
XATTR_NAME_FID, 0, th);
if (rc)
GOTO(out_tx, rc);
/* We don't need a transno for this operation which will be re-executed
* anyway when the OST_WRITE (with a transno assigned) is replayed */
rc = dt_trans_start_local(env, ofd->ofd_osd , th);
if (rc)
GOTO(out_tx, rc);
<----------------- here another thread call osd_attr_set and changed uid/gid
ofd_read_lock(env, ofd_obj);
/* set uid/gid/projid */
if (la->la_valid) {
rc = dt_attr_set(env, dt_obj, la, th);
if (rc)
GOTO(out_unlock, rc);
}
this is actually not the locking broken but rather credits calculation (in optimization part)..
Could you clarify, if we reserve credits for a moment1, and execute operation at moment2, how this relates to calculation optimization? We do it only once per thread.
we'd calculate credits like both UIDs aren't 0 w/o optimization which how it was long before.
At old version ofd_preprw_write takes ofd_read_lock and it protects declare+setattr from OST_SETATTR. Now both operation are separate.
yes, there was a similar issue recently. in general we can't rely on locking anymore as we take locks after declaration (MDD does that from the very beginning).
set_attr for 0 -> 1892 reserved ~290 credits, it is too much to reserve this number every time
not really 
except a tiny local setup...
not really except a tiny local setup...
Alex, having too-large credit reservation hurts concurrency by consuming all journal credits and blocking other threads in start_this_handle(), unless journal is huge. Having a very large journal can consume a lot of RAM, which is also not good. Better to have more efficient journal declarations if possible.
I guess the other question here is why the BRW write clobbers the UID from setattr? That is a problem even more important than the credit calculation, as it may leave objects with the wrong ownership == wrong quota.
yes, I'm aware about credits a bit. in regular setups the journal is really huge as writes (especially fragmented) require more credits and normally number of threads is high as well. where possible it does makes sense to optimize credits, of course (e.g. LU-4611), but we can't change locking - DoM is the point where we had to unify it with MDT's order.
It looks bug don't related to the credits at all, but it related to the brw internal state.
based on Alex comment early
>> ofd_write_attrset(pid 13105) do declaration for 0 -> 0, then another thread(pid 13086) changed uid, and then ofd_write_attrset do ofd_read_lock, setattr.
it mean - brw should skip any attribute changes during write, but it looks, brw tries to restore original owner (as setattr executed).
well, OST_WRITE may and should change credentials as this is the way to initialize UID/GID correctly given objects are precreated with no knowledge who will be using them.
yes, but this change only for the precreated objects aka inode version == 0. while any setattr will change an inode version (including a first write), so this change should be NOP once inode version greater than zero.
We don't use the version to check this, but rather the SUID/SGID bits on the inode, which are set on the inode at precreate time, and cleared when the UID/GID are set. Since there is no lock held across the declare and transaction, it makes sense to reverify these flags in the BRW code under lock before setattr is called. The OST_SETATTR code should also lock the inode at this point for exclusion. That avoids one clobbering the other in a race.
Alexandr Boyko (c17825@cray.com) uploaded a new patch: https://review.whamcloud.com/37117
Subject: LU-13093 osd: fix osd_attr_set race
Project: fs/lustre-release
Branch: master
Current Patch Set: 1
Commit: 6902db97e01f3815455892d8dd70faf75e9d53a6
Oleg Drokin (green@whamcloud.com) merged in patch https://review.whamcloud.com/37117/
Subject: LU-13093 osd: fix osd_attr_set race
Project: fs/lustre-release
Branch: master
Current Patch Set:
Commit: 83ac160bd19bcedaa817a419a3912726b9a3bd9d
Landed for 2.14
Etienne AUJAMES (eaujames@ddn.com) uploaded a new patch: https://review.whamcloud.com/43278
Subject: LU-13093 osd: fix osd_attr_set race
Project: fs/lustre-release
Branch: b2_12
Current Patch Set: 1
Commit: 0179daf3bcd73e45deaa922f333dc30af9353984
still hitting this with racer...