[PATCH v2 4/5] fat: eliminate orphaned inode number allocation

[PATCH v2 4/5] fat: eliminate orphaned inode number allocation

[Namjae Jeon]

From: Namjae Jeon <namjae.jeon@samsung.com>

Maintain a list of inode(i_pos) numbers of orphaned inodes (i.e the
inodes that have been unlinked but still having open file
descriptors).At file/directory creation time, skip using such i_pos
values.Removal of the i_pos from the list is done during inode eviction.

Signed-off-by: Namjae Jeon <namjae.jeon@samsung.com>
Signed-off-by: Ravishankar N <ravi.n1@samsung.com>
Signed-off-by: Amit Sahrawat <a.sahrawat@samsung.com>
---
 fs/fat/dir.c        |    4 +++
 fs/fat/fat.h        |   11 ++++++++
 fs/fat/inode.c      |   77 +++++++++++++++++++++++++++++++++++++++++++++++++++
 fs/fat/namei_vfat.c |   25 ++++++++++++++++-
 4 files changed, 116 insertions(+), 1 deletion(-)

diff --git a/fs/fat/dir.c b/fs/fat/dir.c
index 7b1b9c1..e24fe5e 100644
--- a/fs/fat/dir.c
+++ b/fs/fat/dir.c
@@ -1268,7 +1268,10 @@ int fat_add_entries(struct inode *dir, void *slots, int nr_slots,
 		if (pos >= FAT_MAX_DIR_SIZE)
 			goto error;
 
+
 		if (IS_FREE(de->name)) {
+			if (sbi->options.nfs && fat_entry_busy(sbi, pos, bh))
+				goto free;
 			if (prev != bh) {
 				get_bh(bh);
 				bhs[nr_bhs] = prev = bh;
@@ -1278,6 +1281,7 @@ int fat_add_entries(struct inode *dir, void *slots, int nr_slots,
 			if (free_slots == nr_slots)
 				goto found;
 		} else {
+free:
 			for (i = 0; i < nr_bhs; i++)
 				brelse(bhs[i]);
 			prev = NULL;
diff --git a/fs/fat/fat.h b/fs/fat/fat.h
index 88b6c29..9a53783 100644
--- a/fs/fat/fat.h
+++ b/fs/fat/fat.h
@@ -93,6 +93,9 @@ struct msdos_sb_info {
 
 	spinlock_t dir_hash_lock;
 	struct hlist_head dir_hashtable[FAT_HASH_SIZE];
+
+	spinlock_t ipos_busy_lock;
+	struct list_head *ipos_list_head;
 };
 
 #define FAT_CACHE_VALID	0	/* special case for valid cache */
@@ -128,6 +131,12 @@ struct fat_slot_info {
 	struct buffer_head *bh;
 };
 
+struct ipos_busy_list {
+	struct list_head ipos_list;
+	loff_t i_pos;	/* on disk position for busy directory entry */
+	int nr_slots;
+};
+
 static inline struct msdos_sb_info *MSDOS_SB(struct super_block *sb)
 {
 	return sb->s_fs_info;
@@ -341,6 +350,8 @@ extern void fat_detach(struct inode *inode);
 extern struct inode *fat_iget(struct super_block *sb, loff_t i_pos);
 extern struct inode *fat_build_inode(struct super_block *sb,
 			struct msdos_dir_entry *de, loff_t i_pos);
+extern int fat_entry_busy(struct msdos_sb_info *sbi, loff_t ipos,
+			struct buffer_head *bh);
 extern int fat_sync_inode(struct inode *inode);
 extern int fat_fill_super(struct super_block *sb, void *data, int silent,
 			  int isvfat, void (*setup)(struct super_block *));
diff --git a/fs/fat/inode.c b/fs/fat/inode.c
index 0811339..47ff899 100644
--- a/fs/fat/inode.c
+++ b/fs/fat/inode.c
@@ -291,6 +291,23 @@ static void dir_hash_init(struct super_block *sb)
 		INIT_HLIST_HEAD(&sbi->dir_hashtable[i]);
 }
 
+static int ipos_list_init(struct super_block *sb)
+{
+	struct msdos_sb_info *sbi = MSDOS_SB(sb);
+
+	sbi->ipos_list_head = kzalloc(sizeof(struct ipos_busy_list),
+					GFP_KERNEL);
+	if (!sbi->ipos_list_head) {
+		fat_msg(sb, KERN_ERR,
+			"Failed to allocate memory for ipos list head");
+		return -ENOMEM;
+	}
+	spin_lock_init(&sbi->ipos_busy_lock);
+	INIT_LIST_HEAD(sbi->ipos_list_head);
+
+	return 0;
+}
+
 void fat_attach(struct inode *inode, loff_t i_pos)
 {
 	struct msdos_sb_info *sbi = MSDOS_SB(inode->i_sb);
@@ -477,8 +494,64 @@ out:
 
 EXPORT_SYMBOL_GPL(fat_build_inode);
 
+int fat_entry_busy(struct msdos_sb_info *sbi, loff_t ipos,
+				struct buffer_head *bh)
+{
+	struct list_head *pos, *q;
+	struct ipos_busy_list *plist;
+	int offset;
+	loff_t curpos;
+
+	if (list_empty(sbi->ipos_list_head))
+		return 0;
+
+	offset = (ipos - sizeof(struct msdos_dir_entry)) >> MSDOS_DIR_BITS;
+	curpos = ((bh->b_blocknr) << sbi->dir_per_block_bits) | offset;
+
+	spin_lock(&sbi->ipos_busy_lock);
+	list_for_each_safe(pos, q, sbi->ipos_list_head) {
+		plist = list_entry(pos, struct ipos_busy_list, ipos_list);
+		if (plist) {
+			if ((curpos > (plist->i_pos - plist->nr_slots)) &&
+				(curpos <= plist->i_pos)) {
+				spin_unlock(&sbi->ipos_busy_lock);
+				return 1;
+			}
+		}
+	}
+	spin_unlock(&sbi->ipos_busy_lock);
+	return 0;
+}
+
+static void fat_remove_busy_entry(struct inode *inode)
+{
+
+	struct super_block *sb = inode->i_sb;
+	struct msdos_sb_info *sbi = MSDOS_SB(sb);
+	struct list_head *pos, *q;
+	struct ipos_busy_list *plist;
+
+	spin_lock(&sbi->ipos_busy_lock);
+		list_for_each_safe(pos, q, sbi->ipos_list_head) {
+			plist = list_entry(pos, struct ipos_busy_list,
+					 ipos_list);
+			if (plist) {
+				if (plist->i_pos == MSDOS_I(inode)->i_pos) {
+					list_del(pos);
+					kfree(plist);
+					break;
+				}
+			}
+		}
+	spin_unlock(&sbi->ipos_busy_lock);
+
+}
+
 static void fat_evict_inode(struct inode *inode)
 {
+
+	if (MSDOS_SB(inode->i_sb)->options.nfs)
+		fat_remove_busy_entry(inode);
 	truncate_inode_pages(&inode->i_data, 0);
 	if (!inode->i_nlink) {
 		inode->i_size = 0;
@@ -504,6 +577,8 @@ static void fat_put_super(struct super_block *sb)
 		kfree(sbi->options.iocharset);
 
 	sb->s_fs_info = NULL;
+	if (sbi->options.nfs)
+		kfree(sbi->ipos_list_head);
 	kfree(sbi);
 }
 
@@ -1367,6 +1442,8 @@ int fat_fill_super(struct super_block *sb, void *data, int silent, int isvfat,
 	fat_hash_init(sb);
 	dir_hash_init(sb);
 	fat_ent_access_init(sb);
+	if (sbi->options.nfs && ipos_list_init(sb) < 0)
+		goto out_fail;
 
 	/*
 	 * The low byte of FAT's first entry must have same value with
diff --git a/fs/fat/namei_vfat.c b/fs/fat/namei_vfat.c
index 00d58ea..01020b7 100644
--- a/fs/fat/namei_vfat.c
+++ b/fs/fat/namei_vfat.c
@@ -834,6 +834,26 @@ out:
 	return err;
 }
 
+static void fat_add_busy_entry(struct super_block *sb, struct inode *inode,
+				struct fat_slot_info sinfo)
+{
+	struct msdos_sb_info *sbi = MSDOS_SB(sb);
+	struct ipos_busy_list *unlinked_ipos = NULL;
+
+	if (inode && (atomic_read(&inode->i_count))) {
+		unlinked_ipos = kzalloc(sizeof(struct ipos_busy_list),
+					 GFP_KERNEL);
+		if (unlinked_ipos) {
+			unlinked_ipos->i_pos = sinfo.i_pos;
+			unlinked_ipos->nr_slots = sinfo.nr_slots;
+			spin_lock(&sbi->ipos_busy_lock);
+			list_add_tail(&unlinked_ipos->ipos_list,
+					sbi->ipos_list_head);
+			spin_unlock(&sbi->ipos_busy_lock);
+		}
+	}
+}
+
 static int vfat_unlink(struct inode *dir, struct dentry *dentry)
 {
 	struct inode *inode = dentry->d_inode;
@@ -852,7 +872,10 @@ static int vfat_unlink(struct inode *dir, struct dentry *dentry)
 		goto out;
 	clear_nlink(inode);
 	inode->i_mtime = inode->i_atime = CURRENT_TIME_SEC;
-	fat_detach(inode);
+	if (MSDOS_SB(sb)->options.nfs)
+		fat_add_busy_entry(sb, inode, sinfo);
+	else
+		fat_detach(inode);
 out:
 	unlock_super(sb);
 
-- 
1.7.9.5

Re: [PATCH v2 4/5] fat: eliminate orphaned inode number allocation

[OGAWA Hirofumi]

Namjae Jeon <linkinjeon@gmail.com> writes:

> From: Namjae Jeon <namjae.jeon@samsung.com>
>
> Maintain a list of inode(i_pos) numbers of orphaned inodes (i.e the
> inodes that have been unlinked but still having open file
> descriptors).At file/directory creation time, skip using such i_pos
> values.Removal of the i_pos from the list is done during inode eviction.

What happens if the directory (has busy entries) was completely removed?


And Al's point is important for NFS too. If you want stable ino for NFS,
you never can't change it.

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH v2 4/5] fat: eliminate orphaned inode number allocation

[OGAWA Hirofumi]

OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> writes:

> Namjae Jeon <linkinjeon@gmail.com> writes:
>
>> From: Namjae Jeon <namjae.jeon@samsung.com>
>>
>> Maintain a list of inode(i_pos) numbers of orphaned inodes (i.e the
>> inodes that have been unlinked but still having open file
>> descriptors).At file/directory creation time, skip using such i_pos
>> values.Removal of the i_pos from the list is done during inode eviction.
>
> What happens if the directory (has busy entries) was completely removed?
>
>
> And Al's point is important for NFS too. If you want stable ino for NFS,
> you never can't change it.

s/never can't/never can/
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH v2 4/5] fat: eliminate orphaned inode number allocation

[J. Bruce Fields]

On Wed, Sep 05, 2012 at 02:07:40AM +0900, OGAWA Hirofumi wrote:
> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> writes:
> 
> > Namjae Jeon <linkinjeon@gmail.com> writes:
> >
> >> From: Namjae Jeon <namjae.jeon@samsung.com>
> >>
> >> Maintain a list of inode(i_pos) numbers of orphaned inodes (i.e the
> >> inodes that have been unlinked but still having open file
> >> descriptors).At file/directory creation time, skip using such i_pos
> >> values.Removal of the i_pos from the list is done during inode eviction.
> >
> > What happens if the directory (has busy entries) was completely removed?
> >
> >
> > And Al's point is important for NFS too. If you want stable ino for NFS,
> > you never can't change it.
> 
> s/never can't/never can/

If vfat exports aren't fixable, maybe we should just remove that
feature?

I'm afraid that having unfixable half-working vfat exports is just an
attractive nuisance that causes users and developers to waste their
time....

--b.

Re: [PATCH v2 4/5] fat: eliminate orphaned inode number allocation

[OGAWA Hirofumi]

"J. Bruce Fields" <bfields@fieldses.org> writes:

> On Wed, Sep 05, 2012 at 02:07:40AM +0900, OGAWA Hirofumi wrote:
>> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> writes:
>> 
>> > Namjae Jeon <linkinjeon@gmail.com> writes:
>> >
>> >> From: Namjae Jeon <namjae.jeon@samsung.com>
>> >>
>> >> Maintain a list of inode(i_pos) numbers of orphaned inodes (i.e the
>> >> inodes that have been unlinked but still having open file
>> >> descriptors).At file/directory creation time, skip using such i_pos
>> >> values.Removal of the i_pos from the list is done during inode eviction.
>> >
>> > What happens if the directory (has busy entries) was completely removed?
>> >
>> >
>> > And Al's point is important for NFS too. If you want stable ino for NFS,
>> > you never can't change it.
>> 
>> s/never can't/never can/
>
> If vfat exports aren't fixable, maybe we should just remove that
> feature?
>
> I'm afraid that having unfixable half-working vfat exports is just an
> attractive nuisance that causes users and developers to waste their
> time....

In historically, it was introduced by Neil Brown, when nfs export
interface was rewritten (I'm not sure what was intended).

Personally, I'm ok to remove it though, it is really personal
opinion. The state would be rather I don't have strong opinion to
remove.

Thanks.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH v2 4/5] fat: eliminate orphaned inode number allocation

[J. Bruce Fields]

On Wed, Sep 05, 2012 at 04:02:13AM +0900, OGAWA Hirofumi wrote:
> "J. Bruce Fields" <bfields@fieldses.org> writes:
> 
> > On Wed, Sep 05, 2012 at 02:07:40AM +0900, OGAWA Hirofumi wrote:
> >> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> writes:
> >> 
> >> > Namjae Jeon <linkinjeon@gmail.com> writes:
> >> >
> >> >> From: Namjae Jeon <namjae.jeon@samsung.com>
> >> >>
> >> >> Maintain a list of inode(i_pos) numbers of orphaned inodes (i.e the
> >> >> inodes that have been unlinked but still having open file
> >> >> descriptors).At file/directory creation time, skip using such i_pos
> >> >> values.Removal of the i_pos from the list is done during inode eviction.
> >> >
> >> > What happens if the directory (has busy entries) was completely removed?
> >> >
> >> >
> >> > And Al's point is important for NFS too. If you want stable ino for NFS,
> >> > you never can't change it.
> >> 
> >> s/never can't/never can/
> >
> > If vfat exports aren't fixable, maybe we should just remove that
> > feature?
> >
> > I'm afraid that having unfixable half-working vfat exports is just an
> > attractive nuisance that causes users and developers to waste their
> > time....
> 
> In historically, it was introduced by Neil Brown, when nfs export
> interface was rewritten (I'm not sure what was intended).
> 
> Personally, I'm ok to remove it though, it is really personal
> opinion. The state would be rather I don't have strong opinion to
> remove.

Neil, any opinion?

If we can document circumstances under which nfs exports of fat
filesystems are reliable, fine.

Otherwise I'd rather just be clear that we don't support it.

--b.

Re: [PATCH v2 4/5] fat: eliminate orphaned inode number allocation

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 3302 bytes --]

On Tue, 4 Sep 2012 15:25:09 -0400 "J. Bruce Fields" <bfields@fieldses.org>
wrote:

> On Wed, Sep 05, 2012 at 04:02:13AM +0900, OGAWA Hirofumi wrote:
> > "J. Bruce Fields" <bfields@fieldses.org> writes:
> > 
> > > On Wed, Sep 05, 2012 at 02:07:40AM +0900, OGAWA Hirofumi wrote:
> > >> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> writes:
> > >> 
> > >> > Namjae Jeon <linkinjeon@gmail.com> writes:
> > >> >
> > >> >> From: Namjae Jeon <namjae.jeon@samsung.com>
> > >> >>
> > >> >> Maintain a list of inode(i_pos) numbers of orphaned inodes (i.e the
> > >> >> inodes that have been unlinked but still having open file
> > >> >> descriptors).At file/directory creation time, skip using such i_pos
> > >> >> values.Removal of the i_pos from the list is done during inode eviction.
> > >> >
> > >> > What happens if the directory (has busy entries) was completely removed?
> > >> >
> > >> >
> > >> > And Al's point is important for NFS too. If you want stable ino for NFS,
> > >> > you never can't change it.
> > >> 
> > >> s/never can't/never can/
> > >
> > > If vfat exports aren't fixable, maybe we should just remove that
> > > feature?
> > >
> > > I'm afraid that having unfixable half-working vfat exports is just an
> > > attractive nuisance that causes users and developers to waste their
> > > time....
> > 
> > In historically, it was introduced by Neil Brown, when nfs export
> > interface was rewritten (I'm not sure what was intended).
> > 
> > Personally, I'm ok to remove it though, it is really personal
> > opinion. The state would be rather I don't have strong opinion to
> > remove.
> 
> Neil, any opinion?
> 
> If we can document circumstances under which nfs exports of fat
> filesystems are reliable, fine.
> 
> Otherwise I'd rather just be clear that we don't support it.
> 
> --b.


I think that is important to maintain support for NFS export of VFAT on a
best-effort basis.  We can't provide 100% guarantees of all NFS semantics but
that doesn't prevent it from being of real practical benefit to people.

If the usage pattern is "open/read/close" or "open/write/close" while no
other client is accessing the filesystem and while the server is not under
aggressive memory pressure, then it should work quite reliably.

If you rename files while they are open, have lots of file concurrently open,
allow the server to experience high memory pressure (e.g. reading/writing
multiple files that are bigger than memory) etc etc then things can start to
fail.

VFAT is widely used as a file-transfer protocol.  If you use NFS/VFAT in that
way it works fine.  If you try to use it as a general file access protocol,
that is when you hit problems.

The patch series tries to make inode number stable across reboot.  I think
this is not worth the effort as you won't make VFAT access more reliable,
you'll just make it fail differently.

The only real answer to more reliable NFS access to VFAT is the NFSv4 concept
of volatile file handles.  Unfortunately NFSv4 hasn't yet specified these
with sufficient precision to actually use them.

So if anyone wants to improve VFAT/NFS, I suggest that the first step is to
work with the NFSV4-WG to get an implementable specification.  Good luck with
that.

NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH v2 4/5] fat: eliminate orphaned inode number allocation

[Namjae Jeon]

2012/9/5, OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>:
> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> writes:
>
>> Namjae Jeon <linkinjeon@gmail.com> writes:
>>
>>> From: Namjae Jeon <namjae.jeon@samsung.com>
>>>
>>> Maintain a list of inode(i_pos) numbers of orphaned inodes (i.e the
>>> inodes that have been unlinked but still having open file
>>> descriptors).At file/directory creation time, skip using such i_pos
>>> values.Removal of the i_pos from the list is done during inode eviction.
>>
>> What happens if the directory (has busy entries) was completely removed?
Hi OGAWA.
There are 2 cases:
1. The cluster can be used as a data cluster of a file - In which case
we can write
normally into it.
2. The cluster can be allocated to a new directory. In this case, the busy i_pos
locations can not be used to create new entries until the correspnding
inodes are
evicted.

>>
>>
>> And Al's point is important for NFS too. If you want stable ino for NFS,
>> you never can't change it.
Yes, It is also important, I will explain throughout reply in Al's mail.
Thanks!
>
> s/never can't/never can/
> --
> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
>

Re: [PATCH v2 4/5] fat: eliminate orphaned inode number allocation

[OGAWA Hirofumi]

Namjae Jeon <linkinjeon@gmail.com> writes:

> 2012/9/5, OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>:
>> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> writes:
>>
>>> Namjae Jeon <linkinjeon@gmail.com> writes:
>>>
>>>> From: Namjae Jeon <namjae.jeon@samsung.com>
>>>>
>>>> Maintain a list of inode(i_pos) numbers of orphaned inodes (i.e the
>>>> inodes that have been unlinked but still having open file
>>>> descriptors).At file/directory creation time, skip using such i_pos
>>>> values.Removal of the i_pos from the list is done during inode eviction.
>>>
>>> What happens if the directory (has busy entries) was completely removed?
> Hi OGAWA.
> There are 2 cases:
> 1. The cluster can be used as a data cluster of a file - In which case
> we can write
> normally into it.
> 2. The cluster can be allocated to a new directory. In this case, the busy i_pos
> locations can not be used to create new entries until the correspnding
> inodes are
> evicted.

In the (2) case, it has problem. Directory must not have de->name[0] == 0
in middle of entires.  "0" means the end of directory.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH v2 4/5] fat: eliminate orphaned inode number allocation

[Namjae Jeon]

2012/9/5 OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>:
> Namjae Jeon <linkinjeon@gmail.com> writes:
>
>> 2012/9/5, OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>:
>>> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> writes:
>>>
>>>> Namjae Jeon <linkinjeon@gmail.com> writes:
>>>>
>>>>> From: Namjae Jeon <namjae.jeon@samsung.com>
>>>>>
>>>>> Maintain a list of inode(i_pos) numbers of orphaned inodes (i.e the
>>>>> inodes that have been unlinked but still having open file
>>>>> descriptors).At file/directory creation time, skip using such i_pos
>>>>> values.Removal of the i_pos from the list is done during inode eviction.
>>>>
>>>> What happens if the directory (has busy entries) was completely removed?
>> Hi OGAWA.
>> There are 2 cases:
>> 1. The cluster can be used as a data cluster of a file - In which case
>> we can write
>> normally into it.
>> 2. The cluster can be allocated to a new directory. In this case, the busy i_pos
>> locations can not be used to create new entries until the correspnding
>> inodes are
>> evicted.
>
> In the (2) case, it has problem. Directory must not have de->name[0] == 0
> in middle of entires.  "0" means the end of directory.
Good point! I will fix it on next version of patches.
Thanks.
> --
> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>