From 0c8a955bf6f2c070a462ef5daa756c875463d4b1 Mon Sep 17 00:00:00 2001 From: amulyan13 Date: Tue, 7 Jan 2025 13:11:29 +0000 Subject: [PATCH 1/5] Add is_deleted flag to inode --- turbonfs/inc/fs-handler.h | 12 ++++++++ turbonfs/inc/nfs_inode.h | 5 +++ turbonfs/inc/rpc_task.h | 57 +++++++++++++++++++++++++++++++++- turbonfs/src/nfs_client.cpp | 7 +++-- turbonfs/src/rpc_task.cpp | 61 ++++++++++++++++++++++++++++++++++--- 5 files changed, 135 insertions(+), 7 deletions(-) diff --git a/turbonfs/inc/fs-handler.h b/turbonfs/inc/fs-handler.h index fa70096c..c22b83db 100644 --- a/turbonfs/inc/fs-handler.h +++ b/turbonfs/inc/fs-handler.h @@ -329,6 +329,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 +530,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_inode.h b/turbonfs/inc/nfs_inode.h index 0d46a719..e7415ffb 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..64ca04ec 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,27 @@ struct rename_rpc_task return newname; } + void clear_ino_found() + { + ino_found = false; + } + + void set_ino_to_be_deleted(fuse_ino_t ino_to_del) + { + ino_to_be_deleted = ino_to_del; + ino_found = true; + } + + fuse_ino_t get_ino_to_be_deleted() const + { + return ino_to_be_deleted; + } + + bool is_ino_found() const + { + return ino_found; + } + /* * oldparent_ino/oldname will be non-zero/non-null only for silly rename * triggered by user requested rename operation. @@ -1257,6 +1303,13 @@ 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. + */ + bool ino_found; + fuse_ino_t ino_to_be_deleted; + char *name; char *newname; char *oldname; @@ -2007,6 +2060,7 @@ struct rpc_task void init_unlink(fuse_req *request, fuse_ino_t parent_ino, const char *name, + fuse_ino_t ino, /* ino to be deleted */ bool for_silly_rename); void run_unlink(); @@ -2016,7 +2070,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 /* ino to be deleted */); void run_rmdir(); diff --git a/turbonfs/src/nfs_client.cpp b/turbonfs/src/nfs_client.cpp index 35152575..1f750bd7 100644 --- a/turbonfs/src/nfs_client.cpp +++ b/turbonfs/src/nfs_client.cpp @@ -1274,8 +1274,11 @@ void nfs_client::unlink( bool for_silly_rename) { struct rpc_task *tsk = rpc_task_helper->alloc_rpc_task(FUSE_UNLINK); + struct nfs_inode *parent_inode = get_nfs_inode_from_ino(parent_ino); + struct nfs_inode *inode = parent_inode->lookup(name); - tsk->init_unlink(req, parent_ino, name, for_silly_rename); + tsk->init_unlink(req, parent_ino, name, inode->get_fuse_ino(), + for_silly_rename); tsk->run_unlink(); } @@ -1297,7 +1300,7 @@ void nfs_client::rmdir( inode->invalidate_attribute_cache(); } - tsk->init_rmdir(req, parent_ino, name); + tsk->init_rmdir(req, parent_ino, name, inode->get_fuse_ino()); tsk->run_rmdir(); } diff --git a/turbonfs/src/rpc_task.cpp b/turbonfs/src/rpc_task.cpp index afedb12b..6ba92e52 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(); } @@ -547,6 +551,21 @@ 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.clear_ino_found(); + + if (!silly_rename) + { + /* + * If this is a client initiated rename, set the inode that will + * be marked deleted once the rename completes. + * This will be the inode of the destination if it exists. + */ + struct nfs_inode *parent_inode = get_client()->get_nfs_inode_from_ino(newparent_ino); + struct nfs_inode *inode = parent_inode->lookup(newname); + if (inode) { + rpc_api->rename_task.set_ino_to_be_deleted(inode->get_fuse_ino()); + } + } /* * In case of cross-dir rename, we have to choose between @@ -1489,8 +1508,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 +1534,14 @@ 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. + inode->is_deleted = true; } } task->reply_error(status); @@ -1665,6 +1693,12 @@ void rename_callback( task->rpc_api->rename_task.get_newparent_ino(); silly_rename_inode->is_silly_renamed = true; + /* + * We should no longer be able to get new fds to this inode, + * hence mark it deleted. + */ + silly_rename_inode->is_deleted = true; + /* * Successfully (silly)renamed, hold a ref on the parent directory * inode so that it doesn't go away until we have deleted the @@ -1697,6 +1731,16 @@ void rename_callback( task->get_client()->jukebox_retry(task); } else { if (status == 0) { + /* + * Now that the file is renamed, the old inode should no longer + * be accessible to new open call, hence mark it deleted. + */ + if (!silly_rename && task->rpc_api->rename_task.is_ino_found()) { + task->get_client()->get_nfs_inode_from_ino( + task->rpc_api->rename_task.get_ino_to_be_deleted())->is_deleted + = true; + } + /* * We cannot use UPDATE_INODE_WCC() here as we cannot update our * readdir cache with the newly created file/dir, as the readdir @@ -4745,6 +4789,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 From 76c4cc0ce75cfea3718997c3884467b78f051efa Mon Sep 17 00:00:00 2001 From: amulyan13 Date: Tue, 7 Jan 2025 17:11:26 +0000 Subject: [PATCH 2/5] Fix deadlock --- turbonfs/src/nfs_client.cpp | 14 ++++++++++++++ turbonfs/src/rpc_task.cpp | 17 +++-------------- 2 files changed, 17 insertions(+), 14 deletions(-) diff --git a/turbonfs/src/nfs_client.cpp b/turbonfs/src/nfs_client.cpp index 1f750bd7..9e147ae0 100644 --- a/turbonfs/src/nfs_client.cpp +++ b/turbonfs/src/nfs_client.cpp @@ -1360,6 +1360,20 @@ void nfs_client::rename( tsk->init_rename(req, parent_ino, name, newparent_ino, new_name, silly_rename, silly_rename_ino, oldparent_ino, old_name); + + if (!silly_rename) + { + /* + * If this is a client initiated rename, set the inode that will + * be marked deleted once the rename completes. + * This will be the inode of the destination if it exists. + */ + struct nfs_inode *inode = newparent_inode->lookup(new_name); + if (inode) { + tsk->rpc_api->rename_task.set_ino_to_be_deleted(inode->get_fuse_ino()); + } + } + tsk->run_rename(); } diff --git a/turbonfs/src/rpc_task.cpp b/turbonfs/src/rpc_task.cpp index 6ba92e52..a27bdf81 100644 --- a/turbonfs/src/rpc_task.cpp +++ b/turbonfs/src/rpc_task.cpp @@ -553,20 +553,6 @@ void rpc_task::init_rename(fuse_req *request, rpc_api->rename_task.set_oldname(old_name); rpc_api->rename_task.clear_ino_found(); - if (!silly_rename) - { - /* - * If this is a client initiated rename, set the inode that will - * be marked deleted once the rename completes. - * This will be the inode of the destination if it exists. - */ - struct nfs_inode *parent_inode = get_client()->get_nfs_inode_from_ino(newparent_ino); - struct nfs_inode *inode = parent_inode->lookup(newname); - if (inode) { - rpc_api->rename_task.set_ino_to_be_deleted(inode->get_fuse_ino()); - } - } - /* * In case of cross-dir rename, we have to choose between * old and new dir to have the updated cache. We prefer @@ -1800,6 +1786,9 @@ void rename_callback( struct rpc_task *rename_tsk = task->get_client()->get_rpc_task_helper()->alloc_rpc_task_reserved(FUSE_RENAME); + /* + * TODO: Mark the renamed node as deleted. + */ rename_tsk->init_rename( task->rpc_api->req, task->rpc_api->rename_task.get_oldparent_ino(), From a2f499ee738568bb5b3e562facce45830b7b511b Mon Sep 17 00:00:00 2001 From: amulyan13 Date: Thu, 9 Jan 2025 07:26:47 +0000 Subject: [PATCH 3/5] Refactor --- turbonfs/inc/fs-handler.h | 6 +++++- turbonfs/inc/nfs_client.h | 1 + turbonfs/inc/rpc_task.h | 13 +------------ turbonfs/src/nfs_client.cpp | 33 +++++++++++++++------------------ turbonfs/src/rpc_task.cpp | 37 +++++++++++++++++++------------------ 5 files changed, 41 insertions(+), 49 deletions(-) diff --git a/turbonfs/inc/fs-handler.h b/turbonfs/inc/fs-handler.h index c22b83db..f7906612 100644 --- a/turbonfs/inc/fs-handler.h +++ b/turbonfs/inc/fs-handler.h @@ -271,8 +271,12 @@ static void aznfsc_ll_rename(fuse_req_t req, return; } + 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_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]] diff --git a/turbonfs/inc/nfs_client.h b/turbonfs/inc/nfs_client.h index 38665615..ad21b57b 100644 --- a/turbonfs/inc/nfs_client.h +++ b/turbonfs/inc/nfs_client.h @@ -455,6 +455,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/rpc_task.h b/turbonfs/inc/rpc_task.h index 64ca04ec..68329ee3 100644 --- a/turbonfs/inc/rpc_task.h +++ b/turbonfs/inc/rpc_task.h @@ -1216,15 +1216,9 @@ struct rename_rpc_task return newname; } - void clear_ino_found() - { - ino_found = false; - } - void set_ino_to_be_deleted(fuse_ino_t ino_to_del) { ino_to_be_deleted = ino_to_del; - ino_found = true; } fuse_ino_t get_ino_to_be_deleted() const @@ -1232,11 +1226,6 @@ struct rename_rpc_task return ino_to_be_deleted; } - bool is_ino_found() const - { - return ino_found; - } - /* * oldparent_ino/oldname will be non-zero/non-null only for silly rename * triggered by user requested rename operation. @@ -1307,7 +1296,6 @@ struct rename_rpc_task * This is the inode which will go out of scope once the rename * operation completes. */ - bool ino_found; fuse_ino_t ino_to_be_deleted; char *name; @@ -2125,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 9e147ae0..fea261c0 100644 --- a/turbonfs/src/nfs_client.cpp +++ b/turbonfs/src/nfs_client.cpp @@ -1233,9 +1233,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) { @@ -1328,6 +1337,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, @@ -1359,21 +1369,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); - - if (!silly_rename) - { - /* - * If this is a client initiated rename, set the inode that will - * be marked deleted once the rename completes. - * This will be the inode of the destination if it exists. - */ - struct nfs_inode *inode = newparent_inode->lookup(new_name); - if (inode) { - tsk->rpc_api->rename_task.set_ino_to_be_deleted(inode->get_fuse_ino()); - } - } - + ino_to_mark_deleted, silly_rename, silly_rename_ino, + oldparent_ino, old_name); tsk->run_rename(); } diff --git a/turbonfs/src/rpc_task.cpp b/turbonfs/src/rpc_task.cpp index a27bdf81..e34664fa 100644 --- a/turbonfs/src/rpc_task.cpp +++ b/turbonfs/src/rpc_task.cpp @@ -528,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, @@ -551,7 +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.clear_ino_found(); + rpc_api->rename_task.set_ino_to_be_deleted(ino_to_mark_deleted); + /* * In case of cross-dir rename, we have to choose between @@ -1679,12 +1681,6 @@ void rename_callback( task->rpc_api->rename_task.get_newparent_ino(); silly_rename_inode->is_silly_renamed = true; - /* - * We should no longer be able to get new fds to this inode, - * hence mark it deleted. - */ - silly_rename_inode->is_deleted = true; - /* * Successfully (silly)renamed, hold a ref on the parent directory * inode so that it doesn't go away until we have deleted the @@ -1717,16 +1713,6 @@ void rename_callback( task->get_client()->jukebox_retry(task); } else { if (status == 0) { - /* - * Now that the file is renamed, the old inode should no longer - * be accessible to new open call, hence mark it deleted. - */ - if (!silly_rename && task->rpc_api->rename_task.is_ino_found()) { - task->get_client()->get_nfs_inode_from_ino( - task->rpc_api->rename_task.get_ino_to_be_deleted())->is_deleted - = true; - } - /* * We cannot use UPDATE_INODE_WCC() here as we cannot update our * readdir cache with the newly created file/dir, as the readdir @@ -1755,6 +1741,20 @@ 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. + */ + inode_to_mark_del->is_deleted = true; + } + task->reply_error(status); } else { if (status != 0) { @@ -1794,7 +1794,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(); From b7d00c9f9da96a0681a9a1a30effa1aa40e7b7c9 Mon Sep 17 00:00:00 2001 From: root Date: Thu, 9 Jan 2025 08:55:01 +0000 Subject: [PATCH 4/5] Refactor --- turbonfs/inc/fs-handler.h | 4 ++-- turbonfs/inc/nfs_inode.h | 2 +- turbonfs/inc/rpc_task.h | 4 ++-- turbonfs/src/nfs_client.cpp | 3 ++- turbonfs/src/rpc_task.cpp | 17 ++++++++++++----- 5 files changed, 19 insertions(+), 11 deletions(-) diff --git a/turbonfs/inc/fs-handler.h b/turbonfs/inc/fs-handler.h index f7906612..e162cee5 100644 --- a/turbonfs/inc/fs-handler.h +++ b/turbonfs/inc/fs-handler.h @@ -271,8 +271,8 @@ static void aznfsc_ll_rename(fuse_req_t req, return; } - struct nfs_inode *parent_inode = client->get_nfs_inode_from_ino(parent_ino); - struct nfs_inode *inode = parent_inode->lookup(name); + 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. diff --git a/turbonfs/inc/nfs_inode.h b/turbonfs/inc/nfs_inode.h index e7415ffb..b06f1d10 100644 --- a/turbonfs/inc/nfs_inode.h +++ b/turbonfs/inc/nfs_inode.h @@ -185,7 +185,7 @@ struct nfs_inode /* * This will be marked to true when this inode is deleted. */ - std::atomic is_deleted = 0; + std::atomic is_deleted = 0; /* * Silly rename related info. diff --git a/turbonfs/inc/rpc_task.h b/turbonfs/inc/rpc_task.h index 68329ee3..18e4423d 100644 --- a/turbonfs/inc/rpc_task.h +++ b/turbonfs/inc/rpc_task.h @@ -2048,7 +2048,7 @@ struct rpc_task void init_unlink(fuse_req *request, fuse_ino_t parent_ino, const char *name, - fuse_ino_t ino, /* ino to be deleted */ + fuse_ino_t ino, bool for_silly_rename); void run_unlink(); @@ -2059,7 +2059,7 @@ struct rpc_task void init_rmdir(fuse_req *request, fuse_ino_t parent_ino, const char *name, - fuse_ino_t ino /* ino to be deleted */); + fuse_ino_t ino); void run_rmdir(); diff --git a/turbonfs/src/nfs_client.cpp b/turbonfs/src/nfs_client.cpp index fea261c0..7295cb92 100644 --- a/turbonfs/src/nfs_client.cpp +++ b/turbonfs/src/nfs_client.cpp @@ -412,7 +412,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()); diff --git a/turbonfs/src/rpc_task.cpp b/turbonfs/src/rpc_task.cpp index e34664fa..7e27f21f 100644 --- a/turbonfs/src/rpc_task.cpp +++ b/turbonfs/src/rpc_task.cpp @@ -1448,7 +1448,15 @@ 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(); + 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; + } if (task->get_fuse_req()) { task->reply_error(status); @@ -1529,7 +1537,8 @@ void rmdir_callback( if (inode) { // Now that the dir is removed, mark the inode as deleted. - inode->is_deleted = true; + AZLogDebug("Marking inode {} as deleted", ino); + inode->is_deleted = true; } } task->reply_error(status); @@ -1752,6 +1761,7 @@ void rename_callback( * 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; } @@ -1786,9 +1796,6 @@ void rename_callback( struct rpc_task *rename_tsk = task->get_client()->get_rpc_task_helper()->alloc_rpc_task_reserved(FUSE_RENAME); - /* - * TODO: Mark the renamed node as deleted. - */ rename_tsk->init_rename( task->rpc_api->req, task->rpc_api->rename_task.get_oldparent_ino(), From 2da737ff64fd418f54767a36c4d85b3f6483e0ec Mon Sep 17 00:00:00 2001 From: amulyan13 Date: Fri, 10 Jan 2025 12:42:18 +0000 Subject: [PATCH 5/5] Fix bug --- turbonfs/inc/fs-handler.h | 11 +++++++++-- turbonfs/inc/nfs_client.h | 4 +++- turbonfs/src/nfs_client.cpp | 17 +++++++++-------- turbonfs/src/nfs_inode.cpp | 2 +- turbonfs/src/rpc_task.cpp | 26 +++++++++++++++++--------- 5 files changed, 39 insertions(+), 21 deletions(-) diff --git a/turbonfs/inc/fs-handler.h b/turbonfs/inc/fs-handler.h index e162cee5..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]] diff --git a/turbonfs/inc/nfs_client.h b/turbonfs/inc/nfs_client.h index ad21b57b..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, diff --git a/turbonfs/src/nfs_client.cpp b/turbonfs/src/nfs_client.cpp index 7295cb92..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: @@ -1281,25 +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); - struct nfs_inode *parent_inode = get_nfs_inode_from_ino(parent_ino); - struct nfs_inode *inode = parent_inode->lookup(name); - tsk->init_unlink(req, parent_ino, name, inode->get_fuse_ino(), - 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); @@ -1310,7 +1311,7 @@ void nfs_client::rmdir( inode->invalidate_attribute_cache(); } - tsk->init_rmdir(req, parent_ino, name, inode->get_fuse_ino()); + tsk->init_rmdir(req, parent_ino, name, ino); tsk->run_rmdir(); } 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 7e27f21f..ed4ac6bf 100644 --- a/turbonfs/src/rpc_task.cpp +++ b/turbonfs/src/rpc_task.cpp @@ -1449,14 +1449,22 @@ void unlink_callback( 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(); - 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; - } + // 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()) { task->reply_error(status); @@ -1761,7 +1769,7 @@ void rename_callback( * longer be accessible to new open call, hence mark it * deleted. */ - AZLogDebug("Marking inode {} as deleted", ino_to_del); + AZLogDebug("Marking inode {} as deleted", ino_to_del); inode_to_mark_del->is_deleted = true; }