| Message ID | 20140609200400.1AB405A4A6F@corp2gmr1-2.hot.corp.google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jun 09, 2014 at 01:03:59PM -0700, Andrew Morton wrote: > From: alex chen <alex.chen@huawei.com> > Subject: ocfs2: should add inode into orphan dir after updating entry in ocfs2_rename() > > There are two files a and b in dir /mnt/ocfs2. > node A node B > mv a b > In ocfs2_rename(), after calling > ocfs2_orphan_add(), the inode of > file b will be added into orphan > dir. > > If ocfs2_update_entry() fails, > ocfs2_rename return error and mv > operation fails. But file b still > exists in the parent dir. > > ocfs2_queue_orphan_scan > -> ocfs2_queue_recovery_completion > -> ocfs2_complete_recovery > -> ocfs2_recover_orphans > The inode of the file b will be > put with iput(). > > ocfs2_evict_inode > -> ocfs2_delete_inode > -> ocfs2_wipe_inode > -> ocfs2_remove_inode > OCFS2_VALID_FL in the inode > i_flags will be cleared. > > The file b still can be accessed > on node B. > ls /mnt/ocfs2 > When first read the file b with > ocfs2_read_inode_block(). It will > validate the inode using > ocfs2_validate_inode_block(). > Because OCFS2_VALID_FL not set in > the inode i_flags, so the file > system will be readonly. > > So we should add inode into orphan dir after updating entry in > ocfs2_rename(). That looks fine. I guess the downside now is that if we fail during orphan add we could leak the file which would have been otherwise replaced. In my opinion though this is much better than going readonly because we have a link to a dead file. Thanks for this Alex, --Mark -- Mark Fasheh
On Thu, Jun 12, 2014 at 03:52:14PM -0700, Mark Fasheh wrote: > On Mon, Jun 09, 2014 at 01:03:59PM -0700, Andrew Morton wrote: > > From: alex chen <alex.chen@huawei.com> > > Subject: ocfs2: should add inode into orphan dir after updating entry in ocfs2_rename() > > > > There are two files a and b in dir /mnt/ocfs2. > > node A node B > > mv a b > > In ocfs2_rename(), after calling > > ocfs2_orphan_add(), the inode of > > file b will be added into orphan > > dir. > > > > If ocfs2_update_entry() fails, > > ocfs2_rename return error and mv > > operation fails. But file b still > > exists in the parent dir. > > > > ocfs2_queue_orphan_scan > > -> ocfs2_queue_recovery_completion > > -> ocfs2_complete_recovery > > -> ocfs2_recover_orphans > > The inode of the file b will be > > put with iput(). > > > > ocfs2_evict_inode > > -> ocfs2_delete_inode > > -> ocfs2_wipe_inode > > -> ocfs2_remove_inode > > OCFS2_VALID_FL in the inode > > i_flags will be cleared. > > > > The file b still can be accessed > > on node B. > > ls /mnt/ocfs2 > > When first read the file b with > > ocfs2_read_inode_block(). It will > > validate the inode using > > ocfs2_validate_inode_block(). > > Because OCFS2_VALID_FL not set in > > the inode i_flags, so the file > > system will be readonly. > > > > So we should add inode into orphan dir after updating entry in > > ocfs2_rename(). > > That looks fine. I guess the downside now is that if we fail during orphan > add we could leak the file which would have been otherwise replaced. In my > opinion though this is much better than going readonly because we have a > link to a dead file. > > > Thanks for this Alex, > --Mark Err, probably helps if I leave this, sorry: Reviewed-by: Mark Fasheh <mfasheh@suse.de> -- Mark Fasheh
diff -puN fs/ocfs2/namei.c~ocfs2-should-add-inode-into-orphan-dir-after-updating-entry-in-ocfs2_rename fs/ocfs2/namei.c --- a/fs/ocfs2/namei.c~ocfs2-should-add-inode-into-orphan-dir-after-updating-entry-in-ocfs2_rename +++ a/fs/ocfs2/namei.c @@ -1098,6 +1098,7 @@ static int ocfs2_rename(struct inode *ol struct ocfs2_dir_lookup_result old_entry_lookup = { NULL, }; struct ocfs2_dir_lookup_result orphan_insert = { NULL, }; struct ocfs2_dir_lookup_result target_insert = { NULL, }; + bool should_add_orphan = false; /* At some point it might be nice to break this function up a * bit. */ @@ -1304,6 +1305,7 @@ static int ocfs2_rename(struct inode *ol mlog_errno(status); goto bail; } + should_add_orphan = true; } } else { BUG_ON(new_dentry->d_parent->d_inode != new_dir); @@ -1348,17 +1350,6 @@ static int ocfs2_rename(struct inode *ol goto bail; } - if (S_ISDIR(new_inode->i_mode) || - (ocfs2_read_links_count(newfe) == 1)) { - status = ocfs2_orphan_add(osb, handle, new_inode, - newfe_bh, orphan_name, - &orphan_insert, orphan_dir); - if (status < 0) { - mlog_errno(status); - goto bail; - } - } - /* change the dirent to point to the correct inode */ status = ocfs2_update_entry(new_dir, handle, &target_lookup_res, old_inode); @@ -1373,6 +1364,15 @@ static int ocfs2_rename(struct inode *ol else ocfs2_add_links_count(newfe, -1); ocfs2_journal_dirty(handle, newfe_bh); + if (should_add_orphan) { + status = ocfs2_orphan_add(osb, handle, new_inode, + newfe_bh, orphan_name, + &orphan_insert, orphan_dir); + if (status < 0) { + mlog_errno(status); + goto bail; + } + } } else { /* if the name was not found in new_dir, add it now */ status = ocfs2_add_entry(handle, new_dentry, old_inode,