ocfs2: add functions to add and remove inode in orphan dir

PATCH New akpm at linux-foundation.org 3 participants 8 messages Dec 15, 2014 – Jan 16, 2015 OCFS2

lore

Share & tools

lore

akpm at linux-foundation.org

author

2014-12-15 22:51 UTC #0 lore raw

PATCH 9/15 ocfs2: add functions to add and remove inode in orphan dir

From: Weiwei Wang <[email protected]>
Subject: ocfs2: add functions to add and remove inode in orphan dir

Add functions to add inode to orphan dir and remove inode in orphan dir. Here we do not call ocfs2_prepare_orphan_dir and ocfs2_orphan_add directly. Because append O_DIRECT will add inode to orphan two and may result in more than one orphan entry for the same inode.

[akpm at linux-foundation.org: fix warning]

Signed-off-by Weiwei Wang <[email protected]>

Signed-off-by Joseph Qi <[email protected]>

Cc Joel Becker <[email protected]>

Cc Mark Fasheh <[email protected]>

Signed-off-by Andrew Morton <[email protected]>

---

 fs/ocfs2/journal.h     |    5 
 fs/ocfs2/namei.c       |  315 +++++++++++++++++++++++++++++++++++++++
 fs/ocfs2/namei.h       |    5 
 fs/ocfs2/ocfs2_fs.h    |    5 
 fs/ocfs2/ocfs2_trace.h |    3 
 5 files changed, 332 insertions(+), 1 deletion(-)

fs/ocfs2/journal.h renamed from fs/ocfs2/journal.h~ocfs2-add-functions-to-add-and-remove-inode-in-orphan-dir +5 -0

diff -puN fs/ocfs2/journal.h~ocfs2-add-functions-to-add-and-remove-inode-in-orphan-dir fs/ocfs2/journal.h
@@ -472,6 +472,11 @@ static inline int ocfs2_unlink_credits(s
  * orphan dir index leaf */
 #define OCFS2_DELETE_INODE_CREDITS (3 * OCFS2_INODE_UPDATE_CREDITS + 4)
 
+/* dinode + orphan dir dinode + extent tree leaf block + orphan dir entry +
+ * orphan dir index root + orphan dir index leaf */
+#define OCFS2_INODE_ADD_TO_ORPHAN_CREDITS  (2 * OCFS2_INODE_UPDATE_CREDITS + 4)
+#define OCFS2_INODE_DEL_FROM_ORPHAN_CREDITS  OCFS2_INODE_ADD_TO_ORPHAN_CREDITS
+
 /* dinode update, old dir dinode update, new dir dinode update, old
  * dir dir entry, new dir dir entry, dir entry update for renaming
  * directory + target unlink + 3 x dir index leaves */

fs/ocfs2/namei.c renamed from fs/ocfs2/namei.c~ocfs2-add-functions-to-add-and-remove-inode-in-orphan-dir +315 -0

345 lines not on this page · raw message

fs/ocfs2/namei.h renamed from fs/ocfs2/namei.h~ocfs2-add-functions-to-add-and-remove-inode-in-orphan-dir +5 -0

diff -puN fs/ocfs2/namei.h~ocfs2-add-functions-to-add-and-remove-inode-in-orphan-dir fs/ocfs2/namei.h
@@ -38,6 +38,11 @@ int ocfs2_orphan_del(struct ocfs2_super
 int ocfs2_create_inode_in_orphan(struct inode *dir,
 				 int mode,
 				 struct inode **new_inode);
+int ocfs2_add_inode_to_orphan(struct ocfs2_super *osb,
+		struct inode *inode);
+int ocfs2_del_inode_from_orphan(struct ocfs2_super *osb,
+		struct inode *inode, int update_isize,
+		loff_t end);
 int ocfs2_mv_orphaned_inode_to_new(struct inode *dir,
 				   struct inode *new_inode,
 				   struct dentry *new_dentry);

fs/ocfs2/ocfs2_fs.h renamed from fs/ocfs2/ocfs2_fs.h~ocfs2-add-functions-to-add-and-remove-inode-in-orphan-dir +4 -1

diff -puN fs/ocfs2/ocfs2_fs.h~ocfs2-add-functions-to-add-and-remove-inode-in-orphan-dir fs/ocfs2/ocfs2_fs.h
@@ -229,6 +229,7 @@
 #define OCFS2_CHAIN_FL		(0x00000400)	/* Chain allocator */
 #define OCFS2_DEALLOC_FL	(0x00000800)	/* Truncate log */
 #define OCFS2_QUOTA_FL		(0x00001000)	/* Quota file */
+#define OCFS2_DIO_ORPHANED_FL	(0X00002000)	/* On the orphan list especially for dio */
 
 /*
  * Flags on ocfs2_dinode.i_dyn_features

@@ -729,7 +730,9 @@ struct ocfs2_dinode {
 					   inode belongs to.  Only valid
 					   if allocated from a
 					   discontiguous block group */
-/*A0*/	__le64 i_reserved2[3];
+/*A0*/	__le16 i_dio_orphaned_slot;	/* only used for append dio write */
+	__le16 i_reserved1[3];
+	__le64 i_reserved2[2];
 /*B8*/	union {
 		__le64 i_pad1;		/* Generic way to refer to this
 					   64bit union */

fs/ocfs2/ocfs2_trace.h renamed from fs/ocfs2/ocfs2_trace.h~ocfs2-add-functions-to-add-and-remove-inode-in-orphan-dir +3 -0

diff -puN fs/ocfs2/ocfs2_trace.h~ocfs2-add-functions-to-add-and-remove-inode-in-orphan-dir fs/ocfs2/ocfs2_trace.h
@@ -2373,6 +2373,9 @@ DEFINE_OCFS2_ULL_EVENT(ocfs2_orphan_add_
 
 DEFINE_OCFS2_ULL_UINT_EVENT(ocfs2_orphan_add_end);
 
+DEFINE_OCFS2_ULL_EVENT(ocfs2_dio_orphan_add_begin);
+DEFINE_OCFS2_ULL_UINT_EVENT(ocfs2_dio_orphan_add_end);
+
 TRACE_EVENT(ocfs2_orphan_del,
 	TP_PROTO(unsigned long long dir, const char *name, int namelen),
 	TP_ARGS(dir, name, namelen),

Mark Fasheh

2014-12-18 23:18 UTC #1 lore raw

PATCH 9/15 ocfs2: add functions to add and remove inode in orphan dir

Thanks once again for all of this by the way.

Quoted 7 lines · Andrew Morton

From: Weiwei Wang <[email protected]>
Subject: ocfs2: add functions to add and remove inode in orphan dir
Add functions to add inode to orphan dir and remove inode in orphan dir. Here we do not call ocfs2_prepare_orphan_dir and ocfs2_orphan_add directly. Because append O_DIRECT will add inode to orphan two and may result in more than one orphan entry for the same inode.

Just so I understand - your problem is that ocfs2_prepare_orphan_dir() will EEXIST when it finds the inode already there.

The solution you have below has a couple problems, stemming from the fact that it uses the same name for an inode that is orphaned because of nlink == 0 as that of one that is orphaned because it is undergoing direct IO.

Having identical names in a directory is always considered an error, even for system directories like the orphan dir. In my honest opinion we should not do that here either.

Instead you could use a prefix for those entries - something like "dio-".

If you modify ocfs2_prepare_orphan_dir() and __ocfs2_prepare_orphan_dir() to pass down an optional prefix which is prepended to the name you don't need a special dio prepare function. Then we don't need ocfs2_dio_prepare_orphan_dir() any more.

In fact, the EEXIST check now becomes valid again and you don't need to do all that extra work in ocfs2_dio_orphan_add() to figure out whether the name is there because of nlink == 0 or we're doing dio (or both!).

Actually, it really becomes trivial to special case dio in ocfs2_orphan_add() so I would suggest just adding a boolean argument and doing the bits for dio there, thus allowing us to drop ocfs2_dio_orphan_add() too.

I hope this all helps.

	--Mark

--
Mark Fasheh

Joseph Qi

2014-12-19 03:37 UTC #2 lore raw

PATCH 9/15 ocfs2: add functions to add and remove inode in orphan dir

Hi Mark,
I much appreciate your pertinent comments on this feature.

The reason we use orphan dir is to make sure allocating blocks and updating inode size consistent, and this will result an entry in orphan is not truly deleted one.
To distinguish dio from unlink, we introduce a new dinode flag OCFS2_DIO_ORPHANED_FL.
So after this change, once unlink and dio happen concurrently, there may be two kinds of results:
1. On the same node, only one entry will be added with flag OCFS2_ORPHANED_FL, and flag OCFS2_DIO_ORPHANED_FL will be set and clear during dio.
2. On different nodes, two entries will be added with each flag OCFS2_ORPHANED_FL or OCFS2_DIO_ORPHANED_FL.

Since entry name is stringified from blkno and the length is fixed OCFS2_ORPHAN_NAMELEN, so adding a prefix as you suggested may affect too much.

Quoted 48 lines · Mark Fasheh

Thanks once again for all of this by the way.
Quoted 7 lines · Andrew Morton
From: Weiwei Wang <[email protected]>
Subject: ocfs2: add functions to add and remove inode in orphan dir
Add functions to add inode to orphan dir and remove inode in orphan dir. Here we do not call ocfs2_prepare_orphan_dir and ocfs2_orphan_add directly. Because append O_DIRECT will add inode to orphan two and may result in more than one orphan entry for the same inode.
Just so I understand - your problem is that ocfs2_prepare_orphan_dir() will EEXIST when it finds the inode already there.
The solution you have below has a couple problems, stemming from the fact that it uses the same name for an inode that is orphaned because of nlink == 0 as that of one that is orphaned because it is undergoing direct IO.
Having identical names in a directory is always considered an error, even for system directories like the orphan dir. In my honest opinion we should not do that here either.
Instead you could use a prefix for those entries - something like "dio-".
If you modify ocfs2_prepare_orphan_dir() and __ocfs2_prepare_orphan_dir() to pass down an optional prefix which is prepended to the name you don't need a special dio prepare function. Then we don't need ocfs2_dio_prepare_orphan_dir() any more.
In fact, the EEXIST check now becomes valid again and you don't need to do all that extra work in ocfs2_dio_orphan_add() to figure out whether the name is there because of nlink == 0 or we're doing dio (or both!).
Actually, it really becomes trivial to special case dio in ocfs2_orphan_add() so I would suggest just adding a boolean argument and doing the bits for dio there, thus allowing us to drop ocfs2_dio_orphan_add() too.
I hope this all helps.
	--Mark
--
Mark Fasheh
.

Mark Fasheh

2014-12-19 20:33 UTC #3 lore raw

PATCH 9/15 ocfs2: add functions to add and remove inode in orphan dir

Quoted 6 lines · Joseph Qi

Hi Mark,
I much appreciate your pertinent comments on this feature.
The reason we use orphan dir is to make sure allocating blocks and updating inode size consistent, and this will result an entry in orphan is not truly deleted one.

Right, I got that from the patch. Good idea by the way.

Quoted 2 lines

To distinguish dio from unlink, we introduce a new dinode flag OCFS2_DIO_ORPHANED_FL.

Ok, that's also good.

Quoted 11 lines

So after this change, once unlink and dio happen concurrently, there may be two kinds of results:
1. On the same node, only one entry will be added with flag OCFS2_ORPHANED_FL, and flag OCFS2_DIO_ORPHANED_FL will be set and clear during dio.
2. On different nodes, two entries will be added with each flag OCFS2_ORPHANED_FL or OCFS2_DIO_ORPHANED_FL.
Since entry name is stringified from blkno and the length is fixed OCFS2_ORPHAN_NAMELEN, so adding a prefix as you suggested may affect too much.

No, adding the prefix is much more preferable to intentionally corrupting the conents of a directory. That OCFS2_ORPHAN_NAMELEN is a #define does not matter, you are free to make it whatever reasonable value fits the name. The directory code naturally handles this, whtether the length is OCFS2_ORPHAN_NAMELEN or OCFS2_ORPHAN_NAMELEN + PREFIX_NAMELEN.

Also if you were to run fsck against the file system it will (should) complain about having duplciate entries in the orphan dir.

Aside from the corruption issue, there are usability issues too. Someone running "ls //orphan_dir:0000" will not be able to tell whether a name is there for dio or nlink==0. If you prefix it, then debugging becomes much easier.

Btw, did you see my note about a readonly superblock flag for this? We're going to need to handle backward compatibility too.

	--Mark

--
Mark Fasheh

Joseph Qi

2014-12-22 01:04 UTC #4 lore raw

PATCH 9/15 ocfs2: add functions to add and remove inode in orphan dir

Quoted 35 lines · Mark Fasheh

Quoted 6 lines · Joseph Qi
Hi Mark,
I much appreciate your pertinent comments on this feature.
The reason we use orphan dir is to make sure allocating blocks and updating inode size consistent, and this will result an entry in orphan is not truly deleted one.
Right, I got that from the patch. Good idea by the way.
Quoted 2 lines
To distinguish dio from unlink, we introduce a new dinode flag OCFS2_DIO_ORPHANED_FL.
Ok, that's also good.
Quoted 11 lines
So after this change, once unlink and dio happen concurrently, there may be two kinds of results:
1. On the same node, only one entry will be added with flag OCFS2_ORPHANED_FL, and flag OCFS2_DIO_ORPHANED_FL will be set and clear during dio.
2. On different nodes, two entries will be added with each flag OCFS2_ORPHANED_FL or OCFS2_DIO_ORPHANED_FL.
Since entry name is stringified from blkno and the length is fixed OCFS2_ORPHAN_NAMELEN, so adding a prefix as you suggested may affect too much.
No, adding the prefix is much more preferable to intentionally corrupting the conents of a directory. That OCFS2_ORPHAN_NAMELEN is a #define does not matter, you are free to make it whatever reasonable value fits the name. The directory code naturally handles this, whtether the length is OCFS2_ORPHAN_NAMELEN or OCFS2_ORPHAN_NAMELEN + PREFIX_NAMELEN.

Quoted 8 lines

Also if you were to run fsck against the file system it will (should) complain about having duplciate entries in the orphan dir.
Aside from the corruption issue, there are usability issues too. Someone running "ls //orphan_dir:0000" will not be able to tell whether a name is there for dio or nlink==0. If you prefix it, then debugging becomes much easier.

Okay, I will try to take your advice and make a change.

Quoted 3 lines

Btw, did you see my note about a readonly superblock flag for this? We're going to need to handle backward compatibility too.

Yes. At my first glance, it may need a lot change to support feature ro compat at my first glance. I need to investigate on it before do the actual change. Thanks.

Quoted 7 lines

	--Mark
--
Mark Fasheh
.

Mark Fasheh

2014-12-22 07:06 UTC #5 lore raw

PATCH 9/15 ocfs2: add functions to add and remove inode in orphan dir

Quoted 21 lines · Joseph Qi

Quoted 21 lines
Quoted 11 lines
So after this change, once unlink and dio happen concurrently, there may be two kinds of results:
1. On the same node, only one entry will be added with flag OCFS2_ORPHANED_FL, and flag OCFS2_DIO_ORPHANED_FL will be set and clear during dio.
2. On different nodes, two entries will be added with each flag OCFS2_ORPHANED_FL or OCFS2_DIO_ORPHANED_FL.
Since entry name is stringified from blkno and the length is fixed OCFS2_ORPHAN_NAMELEN, so adding a prefix as you suggested may affect too much.
No, adding the prefix is much more preferable to intentionally corrupting the conents of a directory. That OCFS2_ORPHAN_NAMELEN is a #define does not matter, you are free to make it whatever reasonable value fits the name. The directory code naturally handles this, whtether the length is OCFS2_ORPHAN_NAMELEN or OCFS2_ORPHAN_NAMELEN + PREFIX_NAMELEN.
Also if you were to run fsck against the file system it will (should) complain about having duplciate entries in the orphan dir.

Quoted 6 lines

Quoted 5 lines
Aside from the corruption issue, there are usability issues too. Someone running "ls //orphan_dir:0000" will not be able to tell whether a name is there for dio or nlink==0. If you prefix it, then debugging becomes much easier.
Okay, I will try to take your advice and make a change.

Great, thank you.

Quoted 5 lines

Quoted 2 lines
Btw, did you see my note about a readonly superblock flag for this? We're going to need to handle backward compatibility too.
Yes. At my first glance, it may need a lot change to support feature ro compat at my first glance. I need to investigate on it before do the actual change. Thanks.

Kernel should be straight forward - if the flag isn't there just use the existing fallback. Otherwise we can use the new direct_IO code.

From memory, in ocfs2-tools you'll have to update mkfs to write the flag, tunefs to turn it on/off for an existing filesystem and fsck to truncate the direct-io orphans it finds.

	--Mark

--
Mark Fasheh

Joseph Qi

2015-01-13 08:36 UTC #6 lore raw

PATCH 9/15 ocfs2: add functions to add and remove inode in orphan dir

Hi Mark,

Quoted 38 lines · Mark Fasheh

Quoted 21 lines · Joseph Qi
Quoted 21 lines
Quoted 11 lines
So after this change, once unlink and dio happen concurrently, there may be two kinds of results:
1. On the same node, only one entry will be added with flag OCFS2_ORPHANED_FL, and flag OCFS2_DIO_ORPHANED_FL will be set and clear during dio.
2. On different nodes, two entries will be added with each flag OCFS2_ORPHANED_FL or OCFS2_DIO_ORPHANED_FL.
Since entry name is stringified from blkno and the length is fixed OCFS2_ORPHAN_NAMELEN, so adding a prefix as you suggested may affect too much.
No, adding the prefix is much more preferable to intentionally corrupting the conents of a directory. That OCFS2_ORPHAN_NAMELEN is a #define does not matter, you are free to make it whatever reasonable value fits the name. The directory code naturally handles this, whtether the length is OCFS2_ORPHAN_NAMELEN or OCFS2_ORPHAN_NAMELEN + PREFIX_NAMELEN.
Also if you were to run fsck against the file system it will (should) complain about having duplciate entries in the orphan dir.
Quoted 6 lines
Quoted 5 lines
Aside from the corruption issue, there are usability issues too. Someone running "ls //orphan_dir:0000" will not be able to tell whether a name is there for dio or nlink==0. If you prefix it, then debugging becomes much easier.
Okay, I will try to take your advice and make a change.
Great, thank you.
Quoted 5 lines
Quoted 2 lines
Btw, did you see my note about a readonly superblock flag for this? We're going to need to handle backward compatibility too.
Yes. At my first glance, it may need a lot change to support feature ro compat at my first glance. I need to investigate on it before do the actual change. Thanks.

Quoted 8 lines

Kernel should be straight forward - if the flag isn't there just use the existing fallback. Otherwise we can use the new direct_IO code.
Quoted 1 line
From memory, in ocfs2-tools you'll have to update mkfs to write the flag,
tunefs to turn it on/off for an existing filesystem and fsck to truncate the direct-io orphans it finds.
	--Mark

For the comments about setting it to a ro compat feature, I still have one more questions.
If I check this feature flag in ocfs2_direct_IO, that means the existing formatted ocfs2 volume won't take this feature except we explicitly turn it on using tunefs.
But I don't think there is any problem if feature is default on. Do you mean we have to treat this feature as optional and let user choose?

Quoted 5 lines

--
Mark Fasheh
.

Mark Fasheh

2015-01-16 22:56 UTC #7 lore raw

PATCH 9/15 ocfs2: add functions to add and remove inode in orphan dir

Quoted 49 lines · Joseph Qi

Hi Mark,
Quoted 38 lines · Mark Fasheh
Quoted 21 lines · Joseph Qi
Quoted 21 lines
Quoted 11 lines
So after this change, once unlink and dio happen concurrently, there may be two kinds of results:
1. On the same node, only one entry will be added with flag OCFS2_ORPHANED_FL, and flag OCFS2_DIO_ORPHANED_FL will be set and clear during dio.
2. On different nodes, two entries will be added with each flag OCFS2_ORPHANED_FL or OCFS2_DIO_ORPHANED_FL.
Since entry name is stringified from blkno and the length is fixed OCFS2_ORPHAN_NAMELEN, so adding a prefix as you suggested may affect too much.
No, adding the prefix is much more preferable to intentionally corrupting the conents of a directory. That OCFS2_ORPHAN_NAMELEN is a #define does not matter, you are free to make it whatever reasonable value fits the name. The directory code naturally handles this, whtether the length is OCFS2_ORPHAN_NAMELEN or OCFS2_ORPHAN_NAMELEN + PREFIX_NAMELEN.
Also if you were to run fsck against the file system it will (should) complain about having duplciate entries in the orphan dir.
Quoted 6 lines
Quoted 5 lines
Aside from the corruption issue, there are usability issues too. Someone running "ls //orphan_dir:0000" will not be able to tell whether a name is there for dio or nlink==0. If you prefix it, then debugging becomes much easier.
Okay, I will try to take your advice and make a change.
Great, thank you.
Quoted 5 lines
Quoted 2 lines
Btw, did you see my note about a readonly superblock flag for this? We're going to need to handle backward compatibility too.
Yes. At my first glance, it may need a lot change to support feature ro compat at my first glance. I need to investigate on it before do the actual change. Thanks.
Quoted 8 lines
Kernel should be straight forward - if the flag isn't there just use the existing fallback. Otherwise we can use the new direct_IO code.
Quoted 1 line
From memory, in ocfs2-tools you'll have to update mkfs to write the flag,
tunefs to turn it on/off for an existing filesystem and fsck to truncate the direct-io orphans it finds.
	--Mark

Quoted 5 lines

For the comments about setting it to a ro compat feature, I still have one more questions.
If I check this feature flag in ocfs2_direct_IO, that means the existing formatted ocfs2 volume won't take this feature except we explicitly turn it on using tunefs.

Correct.

Quoted 3 lines

But I don't think there is any problem if feature is default on. Do you mean we have to treat this feature as optional and let user choose?

Yes it should be optional, though we can always have mkfs.ocfs2 turn it on by default.

If the user uses this, and the node crashes with inodes in the orphan dir they might be deleted by another node with an ealier version of the fs code. We can not allow that and therefore have to guard with an incompat bit.

	--Mark

--
Mark Fasheh