diff --git a/turbonfs/inc/fs-handler.h b/turbonfs/inc/fs-handler.h index fa70096c..968f021a 100644 --- a/turbonfs/inc/fs-handler.h +++ b/turbonfs/inc/fs-handler.h @@ -154,6 +154,9 @@ static void aznfsc_ll_unlink(fuse_req_t req, fmt::ptr(req), parent_ino, name); struct nfs_client *client = get_nfs_client_from_fuse_req(req); + struct nfs_inode *parent_inode = client->get_nfs_inode_from_ino(parent_ino); + struct nfs_inode *inode = parent_inode->lookup(name); + const fuse_ino_t ino = inode ? inode->get_fuse_ino() : 0; /* * Call silly_rename() to see if it wants to silly rename instead of unlink. @@ -167,7 +170,7 @@ static void aznfsc_ll_unlink(fuse_req_t req, return; } - client->unlink(req, parent_ino, name, false /* for_silly_rename */); + client->unlink(req, parent_ino, name, ino, false /* for_silly_rename */); } [[maybe_unused]] @@ -181,7 +184,11 @@ static void aznfsc_ll_rmdir(fuse_req_t req, fmt::ptr(req), parent_ino, name); struct nfs_client *client = get_nfs_client_from_fuse_req(req); - client->rmdir(req, parent_ino, name); + struct nfs_inode *parent_inode = client->get_nfs_inode_from_ino(parent_ino); + struct nfs_inode *inode = parent_inode->lookup(name); + const fuse_ino_t ino = inode ? inode->get_fuse_ino() : 0; + + client->rmdir(req, parent_ino, name, ino); } [[maybe_unused]] @@ -271,8 +278,12 @@ static void aznfsc_ll_rename(fuse_req_t req, return; } + struct nfs_inode *newparent_inode = client->get_nfs_inode_from_ino(newparent_ino); + struct nfs_inode *inode = newparent_inode->lookup(newname); + const fuse_ino_t ino_to_del = inode ? inode->get_fuse_ino(): 0; + // Perform user requested rename. - client->rename(req, parent_ino, name, newparent_ino, newname); + client->rename(req, parent_ino, name, newparent_ino, newname, ino_to_del); } [[maybe_unused]] @@ -329,6 +340,12 @@ static void aznfsc_ll_open(fuse_req_t req, // Make sure it's not called for directories. assert(!inode->is_dir()); + /* + * Fuse should not call open() for deleted inode. + * The inode will still be valid till a forget call is made. + */ + assert(!inode->is_deleted); + /* * TODO: See comments in readahead.h, ideally readahead state should be * per file pointer (per open handle) but since fuse doesn't let @@ -524,6 +541,12 @@ static void aznfsc_ll_opendir(fuse_req_t req, assert(inode->is_dir()); + /* + * Fuse should not call opendir() for deleted inode. + * The inode will still be valid till a forget call is made. + */ + assert(!inode->is_deleted); + inode->on_fuse_open(FUSE_OPENDIR); assert(inode->opencnt > 0); diff --git a/turbonfs/inc/nfs_client.h b/turbonfs/inc/nfs_client.h index 38665615..e60731bd 100644 --- a/turbonfs/inc/nfs_client.h +++ b/turbonfs/inc/nfs_client.h @@ -425,12 +425,14 @@ struct nfs_client fuse_req_t req, fuse_ino_t parent_ino, const char *name, + fuse_ino_t ino, bool for_silly_rename); void rmdir( fuse_req_t req, fuse_ino_t parent_ino, - const char* name); + const char* name, + fuse_ino_t ino); void symlink( fuse_req_t req, @@ -455,6 +457,7 @@ struct nfs_client const char *name, fuse_ino_t newparent_ino, const char *new_name, + fuse_ino_t ino_to_mark_deleted = 0, bool silly_rename = false, fuse_ino_t silly_rename_ino = 0, fuse_ino_t oldparent_ino = 0, diff --git a/turbonfs/inc/nfs_inode.h b/turbonfs/inc/nfs_inode.h index 0d46a719..b06f1d10 100644 --- a/turbonfs/inc/nfs_inode.h +++ b/turbonfs/inc/nfs_inode.h @@ -182,6 +182,11 @@ struct nfs_inode */ std::atomic opencnt = 0; + /* + * This will be marked to true when this inode is deleted. + */ + std::atomic is_deleted = 0; + /* * Silly rename related info. * If this inode has been successfully silly renamed, is_silly_renamed will diff --git a/turbonfs/inc/rpc_task.h b/turbonfs/inc/rpc_task.h index 99e6a3d1..18e4423d 100644 --- a/turbonfs/inc/rpc_task.h +++ b/turbonfs/inc/rpc_task.h @@ -996,6 +996,11 @@ struct unlink_rpc_task return parent_ino; } + fuse_ino_t get_ino() const + { + return ino; + } + const char *get_file_name() const { return file_name; @@ -1011,6 +1016,11 @@ struct unlink_rpc_task parent_ino = parent; } + void set_ino(fuse_ino_t _ino) + { + ino = _ino; + } + void set_file_name(const char *name) { file_name = ::strdup(name); @@ -1030,6 +1040,9 @@ struct unlink_rpc_task fuse_ino_t parent_ino; char *file_name; + // Inode being deleted. + fuse_ino_t ino; + /* * Is this unlink task deleting a silly-renamed file when the last opencnt * is dropped? If yes, then we need to drop the extra ref on the parent @@ -1050,6 +1063,11 @@ struct rmdir_rpc_task return dir_name; } + fuse_ino_t get_ino() const + { + return ino; + } + void set_parent_ino(fuse_ino_t parent) { parent_ino = parent; @@ -1060,6 +1078,11 @@ struct rmdir_rpc_task dir_name = ::strdup(name); } + void set_ino(fuse_ino_t dirino) + { + ino = dirino; + } + void release() { ::free(dir_name); @@ -1068,6 +1091,8 @@ struct rmdir_rpc_task private: fuse_ino_t parent_ino; char *dir_name; + // Inode of the directory being deleted. + fuse_ino_t ino; }; struct symlink_rpc_task @@ -1191,6 +1216,16 @@ struct rename_rpc_task return newname; } + void set_ino_to_be_deleted(fuse_ino_t ino_to_del) + { + ino_to_be_deleted = ino_to_del; + } + + fuse_ino_t get_ino_to_be_deleted() const + { + return ino_to_be_deleted; + } + /* * oldparent_ino/oldname will be non-zero/non-null only for silly rename * triggered by user requested rename operation. @@ -1257,6 +1292,12 @@ struct rename_rpc_task fuse_ino_t parent_ino; fuse_ino_t newparent_ino; fuse_ino_t oldparent_ino; + /* + * This is the inode which will go out of scope once the rename + * operation completes. + */ + fuse_ino_t ino_to_be_deleted; + char *name; char *newname; char *oldname; @@ -2007,6 +2048,7 @@ struct rpc_task void init_unlink(fuse_req *request, fuse_ino_t parent_ino, const char *name, + fuse_ino_t ino, bool for_silly_rename); void run_unlink(); @@ -2016,7 +2058,8 @@ struct rpc_task */ void init_rmdir(fuse_req *request, fuse_ino_t parent_ino, - const char *name); + const char *name, + fuse_ino_t ino); void run_rmdir(); @@ -2070,6 +2113,7 @@ struct rpc_task const char *name, fuse_ino_t newparent_ino, const char *newname, + fuse_ino_t ino_to_mark_deleted = 0, bool silly_rename = false, fuse_ino_t silly_rename_ino = 0, fuse_ino_t oldparent_ino = 0, diff --git a/turbonfs/src/nfs_client.cpp b/turbonfs/src/nfs_client.cpp index 35152575..8bcf8fc6 100644 --- a/turbonfs/src/nfs_client.cpp +++ b/turbonfs/src/nfs_client.cpp @@ -360,7 +360,8 @@ void nfs_client::jukebox_runner() js->rpc_api->rmdir_task.get_dir_name()); rmdir(js->rpc_api->req, js->rpc_api->rmdir_task.get_parent_ino(), - js->rpc_api->rmdir_task.get_dir_name()); + js->rpc_api->rmdir_task.get_dir_name(), + js->rpc_api->rmdir_task.get_ino()); break; case FUSE_UNLINK: AZLogWarn("[JUKEBOX REISSUE] unlink(req={}, parent_ino={}, " @@ -372,6 +373,7 @@ void nfs_client::jukebox_runner() unlink(js->rpc_api->req, js->rpc_api->unlink_task.get_parent_ino(), js->rpc_api->unlink_task.get_file_name(), + js->rpc_api->unlink_task.get_ino(), js->rpc_api->unlink_task.get_for_silly_rename()); break; case FUSE_SYMLINK: @@ -412,7 +414,8 @@ void nfs_client::jukebox_runner() js->rpc_api->rename_task.get_name(), js->rpc_api->rename_task.get_newparent_ino(), js->rpc_api->rename_task.get_newname(), - js->rpc_api->rename_task.get_silly_rename(), + js->rpc_api->rename_task.get_ino_to_be_deleted(), + js->rpc_api->rename_task.get_silly_rename(), js->rpc_api->rename_task.get_silly_rename_ino(), js->rpc_api->rename_task.get_oldparent_ino(), js->rpc_api->rename_task.get_oldname()); @@ -1233,9 +1236,18 @@ bool nfs_client::silly_rename( */ inode->opencnt++; - rename(req, parent_ino, name, parent_ino, newname, - true /* silly_rename */, inode->get_fuse_ino(), - oldparent_ino, old_name); + if (rename_triggered_silly_rename) { + rename(req, parent_ino, name, parent_ino, newname, + inode->get_fuse_ino(), + true /* silly_rename */, inode->get_fuse_ino(), + oldparent_ino, old_name); + } + else { + rename(req, parent_ino, name, parent_ino, newname, + 0 /* ino_to_mark_deleted */, + true /* silly_rename */, inode->get_fuse_ino(), + oldparent_ino, old_name); + } return true; } else if (!inode) { @@ -1271,22 +1283,24 @@ void nfs_client::unlink( fuse_req_t req, fuse_ino_t parent_ino, const char* name, + fuse_ino_t ino, bool for_silly_rename) { struct rpc_task *tsk = rpc_task_helper->alloc_rpc_task(FUSE_UNLINK); - tsk->init_unlink(req, parent_ino, name, for_silly_rename); + tsk->init_unlink(req, parent_ino, name, ino, for_silly_rename); tsk->run_unlink(); } void nfs_client::rmdir( fuse_req_t req, fuse_ino_t parent_ino, - const char* name) + const char* name, + fuse_ino_t ino) { struct rpc_task *tsk = rpc_task_helper->alloc_rpc_task(FUSE_RMDIR); struct nfs_inode *parent_inode = get_nfs_inode_from_ino(parent_ino); - struct nfs_inode *inode = parent_inode->lookup(name); + struct nfs_inode *inode = get_nfs_inode_from_ino(ino); if (parent_inode->has_dircache()) { parent_inode->get_dircache()->dnlc_remove(name); @@ -1297,7 +1311,7 @@ void nfs_client::rmdir( inode->invalidate_attribute_cache(); } - tsk->init_rmdir(req, parent_ino, name); + tsk->init_rmdir(req, parent_ino, name, ino); tsk->run_rmdir(); } @@ -1325,6 +1339,7 @@ void nfs_client::rename( const char *name, fuse_ino_t newparent_ino, const char *new_name, + fuse_ino_t ino_to_mark_deleted, bool silly_rename, fuse_ino_t silly_rename_ino, fuse_ino_t oldparent_ino, @@ -1356,7 +1371,8 @@ void nfs_client::rename( } tsk->init_rename(req, parent_ino, name, newparent_ino, new_name, - silly_rename, silly_rename_ino, oldparent_ino, old_name); + ino_to_mark_deleted, silly_rename, silly_rename_ino, + oldparent_ino, old_name); tsk->run_rename(); } diff --git a/turbonfs/src/nfs_inode.cpp b/turbonfs/src/nfs_inode.cpp index 74715fc6..d81f1f55 100644 --- a/turbonfs/src/nfs_inode.cpp +++ b/turbonfs/src/nfs_inode.cpp @@ -1004,7 +1004,7 @@ bool nfs_inode::release(fuse_req_t req) invalidate_attribute_cache(); client->unlink(req, parent_ino, - silly_renamed_name.c_str(), true /* for_silly_rename */); + silly_renamed_name.c_str(), 0, true /* for_silly_rename */); return true; } diff --git a/turbonfs/src/rpc_task.cpp b/turbonfs/src/rpc_task.cpp index afedb12b..ed4ac6bf 100644 --- a/turbonfs/src/rpc_task.cpp +++ b/turbonfs/src/rpc_task.cpp @@ -470,12 +470,14 @@ void rpc_task::init_mkdir(fuse_req *request, void rpc_task::init_unlink(fuse_req *request, fuse_ino_t parent_ino, const char *name, + fuse_ino_t ino, bool for_silly_rename) { assert(get_op_type() == FUSE_UNLINK); set_fuse_req(request); rpc_api->unlink_task.set_parent_ino(parent_ino); rpc_api->unlink_task.set_file_name(name); + rpc_api->unlink_task.set_ino(ino); rpc_api->unlink_task.set_for_silly_rename(for_silly_rename); fh_hash = get_client()->get_nfs_inode_from_ino(parent_ino)->get_crc(); @@ -483,12 +485,14 @@ void rpc_task::init_unlink(fuse_req *request, void rpc_task::init_rmdir(fuse_req *request, fuse_ino_t parent_ino, - const char *name) + const char *name, + fuse_ino_t ino) { assert(get_op_type() == FUSE_RMDIR); set_fuse_req(request); rpc_api->rmdir_task.set_parent_ino(parent_ino); rpc_api->rmdir_task.set_dir_name(name); + rpc_api->rmdir_task.set_ino(ino); fh_hash = get_client()->get_nfs_inode_from_ino(parent_ino)->get_crc(); } @@ -524,6 +528,7 @@ void rpc_task::init_rename(fuse_req *request, const char *name, fuse_ino_t newparent_ino, const char *newname, + fuse_ino_t ino_to_mark_deleted, bool silly_rename, fuse_ino_t silly_rename_ino, fuse_ino_t oldparent_ino, @@ -547,6 +552,8 @@ void rpc_task::init_rename(fuse_req *request, rpc_api->rename_task.set_silly_rename_ino(silly_rename_ino); rpc_api->rename_task.set_oldparent_ino(oldparent_ino); rpc_api->rename_task.set_oldname(old_name); + rpc_api->rename_task.set_ino_to_be_deleted(ino_to_mark_deleted); + /* * In case of cross-dir rename, we have to choose between @@ -1441,6 +1448,22 @@ void unlink_callback( } else { UPDATE_INODE_ATTR(parent_inode, res->REMOVE3res_u.resok.dir_wcc.after); } + + // Now that the file is deleted, mark the inode as deleted. + const fuse_ino_t ino = task->rpc_api->unlink_task.get_ino(); + if (ino) { + struct nfs_inode *inode = + task->get_client()->get_nfs_inode_from_ino(ino); + assert (inode != nullptr); + AZLogDebug("Marking inode {} as deleted", ino); + inode->is_deleted = true; + } else { + /* + * This can happen if the file is being deleted for a silly rename + * scenario in which case we do not set the ino. + */ + assert(for_silly_rename); + } } if (task->get_fuse_req()) { @@ -1489,8 +1512,12 @@ void rmdir_callback( INJECT_JUKEBOX(res, task); #endif - const fuse_ino_t ino = + const fuse_ino_t parent_ino = task->rpc_api->rmdir_task.get_parent_ino(); + struct nfs_inode *parent_inode = + task->get_client()->get_nfs_inode_from_ino(parent_ino); + const fuse_ino_t ino = + task->rpc_api->rmdir_task.get_ino(); struct nfs_inode *inode = task->get_client()->get_nfs_inode_from_ino(ino); const int status = task->status(rpc_status, NFS_STATUS(res)); @@ -1511,9 +1538,15 @@ void rmdir_callback( * readdirectory_cache. */ if (aznfsc_cfg.cache.attr.user.enable) { - UPDATE_INODE_WCC(inode, res->RMDIR3res_u.resok.dir_wcc); + UPDATE_INODE_WCC(parent_inode, res->RMDIR3res_u.resok.dir_wcc); } else { - UPDATE_INODE_ATTR(inode, res->RMDIR3res_u.resok.dir_wcc.after); + UPDATE_INODE_ATTR(parent_inode, res->RMDIR3res_u.resok.dir_wcc.after); + } + + if (inode) { + // Now that the dir is removed, mark the inode as deleted. + AZLogDebug("Marking inode {} as deleted", ino); + inode->is_deleted = true; } } task->reply_error(status); @@ -1725,6 +1758,21 @@ void rename_callback( * For #3 we will need to issue the actual user requested rename, * and we will respond to fuse when that complete. */ + const fuse_ino_t ino_to_del = + task->rpc_api->rename_task.get_ino_to_be_deleted(); + if (ino_to_del){ + struct nfs_inode *inode_to_mark_del = + client->get_nfs_inode_from_ino(ino_to_del); + + /* + * Now that the file is renamed, the old inode should no + * longer be accessible to new open call, hence mark it + * deleted. + */ + AZLogDebug("Marking inode {} as deleted", ino_to_del); + inode_to_mark_del->is_deleted = true; + } + task->reply_error(status); } else { if (status != 0) { @@ -1761,7 +1809,8 @@ void rename_callback( task->rpc_api->rename_task.get_oldparent_ino(), task->rpc_api->rename_task.get_oldname(), parent_ino, - task->rpc_api->rename_task.get_name()); + task->rpc_api->rename_task.get_name(), + task->rpc_api->rename_task.get_ino_to_be_deleted()); rename_tsk->run_rename(); @@ -4745,6 +4794,15 @@ void rpc_task::send_readdir_or_readdirplus_response( assert(it->nfs_inode->lookupcnt > 0); assert(it->nfs_inode->forget_expected > 0); + /* + * Readdirplus call should not return a deleted inode. + * This can happen if the server returns us stale entry from it's + * readdir cache. + * TODO: This should be fixed. + * https://msazure.visualstudio.com/One/_workitems/edit/30621510 + */ + assert(!it->nfs_inode->is_deleted); + struct fuse_entry_param fuseentry; #ifdef ENABLE_PARANOID