[LU-1154] CLIO does not send parent FID with OBD_MD_FLFID to OST on setattr Created: 29/Feb/12 Updated: 22/Jan/15 Resolved: 22/Jan/15 |
|
| Status: | Resolved |
| Project: | Lustre |
| Component/s: | None |
| Affects Version/s: | Lustre 2.1.0, Lustre 2.2.0 |
| Fix Version/s: | Lustre 2.7.0 |
| Type: | Bug | Priority: | Critical |
| Reporter: | Andreas Dilger | Assignee: | Zhenyu Xu |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | HB | ||
| Story Points: | 3 |
| Severity: | 3 |
| Rank (Obsolete): | 9027 |
| Description |
|
A filter_fid EA should be added to an OST object whenever the object is being modified - either its data or metadata. However, current 2.x clients send filter_fid/OBD_MD_FLFID information only with OST_WRITE RPCs. In contrast, 1.x clients pack the OBD_MD_FLFID information into setattr(mtime) and truncate RPCs. Ideally, the client will pass this information with every object RPC (read or write) so that the OST can verify that the client is accessing the correct OST object for its file IO (most importantly as a sanity check, but also a simple form of capability check). This will also be important for LFSCK Phase II, when the OST needs to verify that the object parent FID matches the MDT inode FID. While there is some code in master client that appears to be sending this information to the OST, the twisty mess that is CLIO appears to lose this information between where the inode is available in ll_setattr_raw(): ll_setattr_raw()
->ll_setattr_ost()
->cl_setattr_ost() # attributes are packed into "ost_lvb" here
->cl_io_loop()
->cl_io_start()
->lov_io_start()
->osc_io_setattr_start()
->cl_object_attr_set()
->osc_attr_set() # attributes unpacked from ost_lvb
->osc_setattr_async_base() # RPC is sent here
but when the RPC is formed in the parent FID is not in the obdo. One might mistakenly assume that since ccc_req_attr_set() calls obdo_from_inode() (which always copies the OBD_MD_FLFID information when the inode FID is passed) that this data would make it into the setattr RPC, but this doesn't appear in the debug logs, nor is the "fid" xattr set on a file with just "touch". There are many twisty functions, all of them look alike, which makes understanding and debugging this code more difficult than it needs to be: cl_setattr_ost() cl_object_attr_set() cl_req_attr_set() osc_req_attr_set() osc_attr_set() ccc_attr_set() ccc_req_attr_set() lov_attr_set() lovsub_req_attr_set() lovsub_attr_set() Looking at callchain for ccc_req_attr_set() (starting from the bottom and reverse engineering the callers): ->osc_enter_cache()
->osc_check_rpcs()
->osc_check_rpcs0()
->osc_send_oap_rpc()
->osc_build_req()
->cl_req_attr_set()
->ccc_req_attr_set()
->obdo_from_inode()
it appears that this function is only used by write and not setattr. It may be possible to use something like the following: obdo_from_inode(oa, NULL, &cl_i2info(ccc_object_inode(obj))->lli_fid, 0); It would be nice to remove the duplication of "cl_attr_set" and "cl_req_attr_set", but I'm not well enough versed in this code to understand the consequences yet. I think the following functions are no longer used by the master client, but I'm not really sure. It may be that even the MDS does not use them anymore on the orion branch, so they might be removed to reduce confusion and obsolete code. lov_setattr() lov_setattr_async() lov_prep_setattr_set() lov_update_setattr_set() lov_fini_setattr_set() cb_setattr_set() osc_setattr() osc_setattr_async() osc_setattr_interpret() osc_setattr_async_base() osc_io_setattr_start() osc_io_setattr_end() osc_setattr_upcall() |
| Comments |
| Comment by Jinshan Xiong (Inactive) [ 01/Mar/12 ] |
|
If you want to send more attribute info to OST in setattr request, we can pass that info into u.ci_setattr so that osc can use it to pack RPCs. For callbacks of cl_req, sometimes we need to fetch some information from upper layers when we clean up cache(BRW) on the osc layer. Other similar callbacks are osc_ {refresh_count|make_ready|completion}. I agree it's very bad to have obsoleted code in clio stack. I'll remove them soon. |
| Comment by Andreas Dilger [ 10/Jul/13 ] |
|
Assign this to Bobijam so it can be part of the CLIO cleanup work being done. Sending the MDS FID along with every OST RPC (both read and write, getattr and setattr, punch, destroy, etc) is the most desirable outcome. Not only does this allow the MDS FID to be stored on the OST object (for any modifying RPC), but it also allows the OST to verify that the client is accessing the correct object during normal operation. |
| Comment by Jodi Levi (Inactive) [ 18/Mar/14 ] |
|
Jinshan, |
| Comment by Andreas Dilger [ 18/Mar/14 ] |
|
Jinshan, Bobijam, has this problem been fixed on master, or does work still need to be done for this? I think that this is important for correct LFSCK operation. |
| Comment by Jinshan Xiong (Inactive) [ 19/Mar/14 ] |
|
We don't do this work yet. Right now CLIO only packs OST FID in both WRITE and SETATTR RPC, but never pack parent FID in the RPC. |
| Comment by Andreas Dilger [ 28/Nov/14 ] |
|
Is this part of the CLIO cleanup? This should definitely be fixed, since the lack of parent FID prevents LFSCK from working correctly in some cases, and in the future the OSTs will refuse RPCs that are not correctly identified with the parent MDS FID. |
| Comment by Gerrit Updater [ 01/Dec/14 ] |
|
Bobi Jam (bobijam@gmail.com) uploaded a new patch: http://review.whamcloud.com/12888 |
| Comment by Gerrit Updater [ 02/Dec/14 ] |
|
Bobi Jam (bobijam@gmail.com) uploaded a new patch: http://review.whamcloud.com/12902 |
| Comment by Jinshan Xiong (Inactive) [ 19/Jan/15 ] |
|
the patches are ready to land. |
| Comment by Gerrit Updater [ 22/Jan/15 ] |
|
Oleg Drokin (oleg.drokin@intel.com) merged in patch http://review.whamcloud.com/12888/ |
| Comment by Gerrit Updater [ 22/Jan/15 ] |
|
Oleg Drokin (oleg.drokin@intel.com) merged in patch http://review.whamcloud.com/12902/ |
| Comment by Peter Jones [ 22/Jan/15 ] |
|
Landed for 2.7 |