[PATCH v3 2/5] fat: allocate persistent inode numbers

[PATCH v3 2/5] fat: allocate persistent inode numbers

[Namjae Jeon]

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

All the files on a FAT partition have an on-disk directory entry.
The location of these entries, i_pos, is unique and is constructed by
the fat_make_i_pos() function.We can use this as the inode number making
it persistent across remounts.

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/inode.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/fat/inode.c b/fs/fat/inode.c
index 2689ef5..c3dad9b 100644
--- a/fs/fat/inode.c
+++ b/fs/fat/inode.c
@@ -458,7 +458,10 @@ struct inode *fat_build_inode(struct super_block *sb,
 		inode = ERR_PTR(-ENOMEM);
 		goto out;
 	}
-	inode->i_ino = iunique(sb, MSDOS_ROOT_INO);
+	if (MSDOS_SB(sb)->options.nfs == FAT_NFS_LIMITED)
+		inode->i_ino = i_pos;
+	else
+		inode->i_ino = iunique(sb, MSDOS_ROOT_INO);
 	inode->i_version = 1;
 	err = fat_fill_inode(inode, de);
 	if (err) {
-- 
1.7.9.5

Re: [PATCH v3 2/5] fat: allocate persistent inode numbers

[OGAWA Hirofumi]

Namjae Jeon <linkinjeon@gmail.com> writes:

> -	inode->i_ino = iunique(sb, MSDOS_ROOT_INO);
> +	if (MSDOS_SB(sb)->options.nfs == FAT_NFS_LIMITED)
> +		inode->i_ino = i_pos;
> +	else
> +		inode->i_ino = iunique(sb, MSDOS_ROOT_INO);
>  	inode->i_version = 1;
>  	err = fat_fill_inode(inode, de);
>  	if (err) {

I think we don't need this. Because FH and ino is not necessary to have
relation.

Can we re-introduce ->encode_fh() handler, and export i_pos again?  With
this, I think we can get i_pos correctly. Otherwise, ino may not contain
all bits of i_pos.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH v3 2/5] fat: allocate persistent inode numbers

[Namjae Jeon]

2012/9/22, OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>:
> Namjae Jeon <linkinjeon@gmail.com> writes:
>
>> -	inode->i_ino = iunique(sb, MSDOS_ROOT_INO);
>> +	if (MSDOS_SB(sb)->options.nfs == FAT_NFS_LIMITED)
>> +		inode->i_ino = i_pos;
>> +	else
>> +		inode->i_ino = iunique(sb, MSDOS_ROOT_INO);
>>  	inode->i_version = 1;
>>  	err = fat_fill_inode(inode, de);
>>  	if (err) {
>
> I think we don't need this. Because FH and ino is not necessary to have
> relation.
>
> Can we re-introduce ->encode_fh() handler, and export i_pos again?  With
> this, I think we can get i_pos correctly. Otherwise, ino may not contain
> all bits of i_pos.
I already tried to fix this issue using encode_fh without stable ino before.
But I reached conclusion that we should use stable inode number.

e.g. If we rebuild inode number using i_pos of fh, inode number is
changed by i_unique.
And It is not match with inode number of FH on NFS client. So estale
error will happen.

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

Re: [PATCH v3 2/5] fat: allocate persistent inode numbers

[OGAWA Hirofumi]

Namjae Jeon <linkinjeon@gmail.com> writes:

>> I think we don't need this. Because FH and ino is not necessary to have
>> relation.
>>
>> Can we re-introduce ->encode_fh() handler, and export i_pos again?  With
>> this, I think we can get i_pos correctly. Otherwise, ino may not contain
>> all bits of i_pos.
> I already tried to fix this issue using encode_fh without stable ino before.
> But I reached conclusion that we should use stable inode number.
>
> e.g. If we rebuild inode number using i_pos of fh, inode number is
> changed by i_unique.
> And It is not match with inode number of FH on NFS client. So estale
> error will happen.

What is problem if i_ino + i_generation is not match? I think, even if
those didn't match, i_pos in FH should resolve issue, no?
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH v3 2/5] fat: allocate persistent inode numbers

[Namjae Jeon]

2012/9/24, OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>:
> Namjae Jeon <linkinjeon@gmail.com> writes:
>
>>> I think we don't need this. Because FH and ino is not necessary to have
>>> relation.
>>>
>>> Can we re-introduce ->encode_fh() handler, and export i_pos again?  With
>>> this, I think we can get i_pos correctly. Otherwise, ino may not contain
>>> all bits of i_pos.
>> I already tried to fix this issue using encode_fh without stable ino
>> before.
>> But I reached conclusion that we should use stable inode number.
>>
>> e.g. If we rebuild inode number using i_pos of fh, inode number is
>> changed by i_unique.
>> And It is not match with inode number of FH on NFS client. So estale
>> error will happen.
>
> What is problem if i_ino + i_generation is not match? I think, even if
> those didn't match, i_pos in FH should resolve issue, no?
No, It can not resolve issue.
in NFS file handle, there is a reference to the current inode number.
So, if by eviction that is changed - that it will results in "file id
changed" error.
even though using the i_pos we can reconstruct and get the INODE on
the Server, but the NFS handle is no more valid. As the inode number
is also changed, iunique() for the file will result in different
number this time.

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

Re: [PATCH v3 2/5] fat: allocate persistent inode numbers

[OGAWA Hirofumi]

Namjae Jeon <linkinjeon@gmail.com> writes:

>> What is problem if i_ino + i_generation is not match? I think, even if
>> those didn't match, i_pos in FH should resolve issue, no?
> No, It can not resolve issue.
> in NFS file handle, there is a reference to the current inode number.
> So, if by eviction that is changed - that it will results in "file id
> changed" error.

Who returns error?

> even though using the i_pos we can reconstruct and get the INODE on
> the Server, but the NFS handle is no more valid. As the inode number
> is also changed, iunique() for the file will result in different
> number this time.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH v3 2/5] fat: allocate persistent inode numbers

[OGAWA Hirofumi]

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

> Namjae Jeon <linkinjeon@gmail.com> writes:
>
>>> What is problem if i_ino + i_generation is not match? I think, even if
>>> those didn't match, i_pos in FH should resolve issue, no?
>> No, It can not resolve issue.
>> in NFS file handle, there is a reference to the current inode number.
>> So, if by eviction that is changed - that it will results in "file id
>> changed" error.
>
> Who returns error?

This means - which code returns error?

>> even though using the i_pos we can reconstruct and get the INODE on
>> the Server, but the NFS handle is no more valid. As the inode number
>> is also changed, iunique() for the file will result in different
>> number this time.

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

Re: [PATCH v3 2/5] fat: allocate persistent inode numbers

[Namjae Jeon]

2012/9/24, OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>:
> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp> writes:
>
>> Namjae Jeon <linkinjeon@gmail.com> writes:
>>
>>>> What is problem if i_ino + i_generation is not match? I think, even if
>>>> those didn't match, i_pos in FH should resolve issue, no?
>>> No, It can not resolve issue.
>>> in NFS file handle, there is a reference to the current inode number.
>>> So, if by eviction that is changed - that it will results in "file id
>>> changed" error.
>>
>> Who returns error?
>
> This means - which code returns error?
Sorry, My explanation may be insufficient.

nfs_update_inode()-> on NFS client
if ((fattr->valid & NFS_ATTR_FATTR_FILEID) && nfsi->fileid != fattr->fileid) {
		printk(KERN_ERR "NFS: server %s error: fileid changed\n"
			"fsid %s: expected fileid 0x%Lx, got 0x%Lx\n",
			NFS_SERVER(inode)->nfs_client->cl_hostname,
			inode->i_sb->s_id, (long long)nfsi->fileid,
			(long long)fattr->fileid);
		goto out_err;
	}

that the problem in that case will occur with the file handle on NFS
Client because the file handle on NFS client is still referring the
old inode number even though the i-pos is same

Thanks.

>
>>> even though using the i_pos we can reconstruct and get the INODE on
>>> the Server, but the NFS handle is no more valid. As the inode number
>>> is also changed, iunique() for the file will result in different
>>> number this time.
>
> --
> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
>

Re: [PATCH v3 2/5] fat: allocate persistent inode numbers

[OGAWA Hirofumi]

Namjae Jeon <linkinjeon@gmail.com> writes:

>> This means - which code returns error?
> Sorry, My explanation may be insufficient.
>
> nfs_update_inode()-> on NFS client
> if ((fattr->valid & NFS_ATTR_FATTR_FILEID) && nfsi->fileid != fattr->fileid) {
> 		printk(KERN_ERR "NFS: server %s error: fileid changed\n"
> 			"fsid %s: expected fileid 0x%Lx, got 0x%Lx\n",
> 			NFS_SERVER(inode)->nfs_client->cl_hostname,
> 			inode->i_sb->s_id, (long long)nfsi->fileid,
> 			(long long)fattr->fileid);
> 		goto out_err;
> 	}
>
> that the problem in that case will occur with the file handle on NFS
> Client because the file handle on NFS client is still referring the
> old inode number even though the i-pos is same

I see. fileid seems to be stat.ino on nfsd4. inode->i_ino is actually
just a hash key of inode hash (exception is only in audit, iirc).

So, what happens if we set "stat->ino = i_pos" on fat_getattr().

int fat_getattr(struct vfsmount *mnt, struct dentry *dentry, struct kstat *stat)
{
	struct inode *inode = dentry->d_inode;
	generic_fillattr(inode, stat);
	stat->blksize = MSDOS_SB(inode->i_sb)->cluster_size;
	if (opts->nfs == FAT_NFS_LIMITED) {
		/* Use i_pos for ino. This is used as fileid of nfs. */
		stat->ino = MSDOS_SB(inode->i_sb)->i_pos;
	}
	return 0;
}
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH v3 2/5] fat: allocate persistent inode numbers

[Namjae Jeon]

2012/9/24, OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>:
> Namjae Jeon <linkinjeon@gmail.com> writes:
>
>>> This means - which code returns error?
>> Sorry, My explanation may be insufficient.
>>
>> nfs_update_inode()-> on NFS client
>> if ((fattr->valid & NFS_ATTR_FATTR_FILEID) && nfsi->fileid !=
>> fattr->fileid) {
>> 		printk(KERN_ERR "NFS: server %s error: fileid changed\n"
>> 			"fsid %s: expected fileid 0x%Lx, got 0x%Lx\n",
>> 			NFS_SERVER(inode)->nfs_client->cl_hostname,
>> 			inode->i_sb->s_id, (long long)nfsi->fileid,
>> 			(long long)fattr->fileid);
>> 		goto out_err;
>> 	}
>>
>> that the problem in that case will occur with the file handle on NFS
>> Client because the file handle on NFS client is still referring the
>> old inode number even though the i-pos is same
>
> I see. fileid seems to be stat.ino on nfsd4. inode->i_ino is actually
> just a hash key of inode hash (exception is only in audit, iirc).
>
> So, what happens if we set "stat->ino = i_pos" on fat_getattr().
>
> int fat_getattr(struct vfsmount *mnt, struct dentry *dentry, struct kstat
> *stat)
> {
> 	struct inode *inode = dentry->d_inode;
> 	generic_fillattr(inode, stat);
> 	stat->blksize = MSDOS_SB(inode->i_sb)->cluster_size;
> 	if (opts->nfs == FAT_NFS_LIMITED) {
> 		/* Use i_pos for ino. This is used as fileid of nfs. */
> 		stat->ino = MSDOS_SB(inode->i_sb)->i_pos;
> 	}
> 	return 0;
> }
Okay, Thanks for your help.
I will share the result after checking.

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

Re: [PATCH v3 2/5] fat: allocate persistent inode numbers

[OGAWA Hirofumi]

Namjae Jeon <linkinjeon@gmail.com> writes:

>> I see. fileid seems to be stat.ino on nfsd4. inode->i_ino is actually
>> just a hash key of inode hash (exception is only in audit, iirc).
>>
>> So, what happens if we set "stat->ino = i_pos" on fat_getattr().
>>
>> int fat_getattr(struct vfsmount *mnt, struct dentry *dentry, struct kstat
>> *stat)
>> {
>> 	struct inode *inode = dentry->d_inode;
>> 	generic_fillattr(inode, stat);
>> 	stat->blksize = MSDOS_SB(inode->i_sb)->cluster_size;
>> 	if (opts->nfs == FAT_NFS_LIMITED) {
>> 		/* Use i_pos for ino. This is used as fileid of nfs. */
>> 		stat->ino = MSDOS_SB(inode->i_sb)->i_pos;

		stat->ino = fat_i_pos_read(MSDOS_SB(inode->i_sb), inode);

Ouch, I forgot to use fat_i_pos_read().

>> 	}
>> 	return 0;
>> }
> Okay, Thanks for your help.
> I will share the result after checking.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH v3 2/5] fat: allocate persistent inode numbers

[Namjae Jeon]

2012/9/24, OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>:
> Namjae Jeon <linkinjeon@gmail.com> writes:
>
>>> I see. fileid seems to be stat.ino on nfsd4. inode->i_ino is actually
>>> just a hash key of inode hash (exception is only in audit, iirc).
>>>
>>> So, what happens if we set "stat->ino = i_pos" on fat_getattr().
>>>
>>> int fat_getattr(struct vfsmount *mnt, struct dentry *dentry, struct
>>> kstat
>>> *stat)
>>> {
>>> 	struct inode *inode = dentry->d_inode;
>>> 	generic_fillattr(inode, stat);
>>> 	stat->blksize = MSDOS_SB(inode->i_sb)->cluster_size;
>>> 	if (opts->nfs == FAT_NFS_LIMITED) {
>>> 		/* Use i_pos for ino. This is used as fileid of nfs. */
>>> 		stat->ino = MSDOS_SB(inode->i_sb)->i_pos;
>
> 		stat->ino = fat_i_pos_read(MSDOS_SB(inode->i_sb), inode);
>
> Ouch, I forgot to use fat_i_pos_read().
>
There is some unclear thing.
When I see first mail, I think maybe you don't want to use i_pos for inode->ino.
FAT allocate inode->ino from i_unique on server side and If NFS client
use i_pos for inode->ino in fat_get_attr, inode numbers on each
client/server will still be mismatched.

Would you plz give me hint ?

Thanks OGAWA~.
>>> 	}
>>> 	return 0;
>>> }
>> Okay, Thanks for your help.
>> I will share the result after checking.
> --
> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
>

Re: [PATCH v3 2/5] fat: allocate persistent inode numbers

[OGAWA Hirofumi]

Namjae Jeon <linkinjeon@gmail.com> writes:

> 2012/9/24, OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>:
>> Namjae Jeon <linkinjeon@gmail.com> writes:
>>
>>>> I see. fileid seems to be stat.ino on nfsd4. inode->i_ino is actually
>>>> just a hash key of inode hash (exception is only in audit, iirc).
>>>>
>>>> So, what happens if we set "stat->ino = i_pos" on fat_getattr().
>>>>
>>>> int fat_getattr(struct vfsmount *mnt, struct dentry *dentry, struct
>>>> kstat
>>>> *stat)
>>>> {
>>>> 	struct inode *inode = dentry->d_inode;
>>>> 	generic_fillattr(inode, stat);
>>>> 	stat->blksize = MSDOS_SB(inode->i_sb)->cluster_size;
>>>> 	if (opts->nfs == FAT_NFS_LIMITED) {
>>>> 		/* Use i_pos for ino. This is used as fileid of nfs. */
>>>> 		stat->ino = MSDOS_SB(inode->i_sb)->i_pos;
>>
>> 		stat->ino = fat_i_pos_read(MSDOS_SB(inode->i_sb), inode);
>>
>> Ouch, I forgot to use fat_i_pos_read().
>>
> There is some unclear thing.
> When I see first mail, I think maybe you don't want to use i_pos for inode->ino.
> FAT allocate inode->ino from i_unique on server side and If NFS client
> use i_pos for inode->ino in fat_get_attr, inode numbers on each
> client/server will still be mismatched.
>
> Would you plz give me hint ?

->i_ino is long. It can't hold i_pos fully on 32bit arch, so we can't
use ->i_no to store i_pos, and changing ->i_ino is unnecessary. If
getattr() returned i_pos as ino, nobody see ->i_ino anymore except
internal of kernel.

Furthermore I think there is no issue even if server and client didn't
have same ino. Because client just uses FH (nfs4 seems to be using
stat.ino though).
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH v3 2/5] fat: allocate persistent inode numbers

[Namjae Jeon]

2012/9/24, OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>:
> Namjae Jeon <linkinjeon@gmail.com> writes:
>
>> 2012/9/24, OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>:
>>> Namjae Jeon <linkinjeon@gmail.com> writes:
>>>
>>>>> I see. fileid seems to be stat.ino on nfsd4. inode->i_ino is actually
>>>>> just a hash key of inode hash (exception is only in audit, iirc).
>>>>>
>>>>> So, what happens if we set "stat->ino = i_pos" on fat_getattr().
>>>>>
>>>>> int fat_getattr(struct vfsmount *mnt, struct dentry *dentry, struct
>>>>> kstat
>>>>> *stat)
>>>>> {
>>>>> 	struct inode *inode = dentry->d_inode;
>>>>> 	generic_fillattr(inode, stat);
>>>>> 	stat->blksize = MSDOS_SB(inode->i_sb)->cluster_size;
>>>>> 	if (opts->nfs == FAT_NFS_LIMITED) {
>>>>> 		/* Use i_pos for ino. This is used as fileid of nfs. */
>>>>> 		stat->ino = MSDOS_SB(inode->i_sb)->i_pos;
>>>
>>> 		stat->ino = fat_i_pos_read(MSDOS_SB(inode->i_sb), inode);
>>>
>>> Ouch, I forgot to use fat_i_pos_read().
>>>
>> There is some unclear thing.
>> When I see first mail, I think maybe you don't want to use i_pos for
>> inode->ino.
>> FAT allocate inode->ino from i_unique on server side and If NFS client
>> use i_pos for inode->ino in fat_get_attr, inode numbers on each
>> client/server will still be mismatched.
>>
>> Would you plz give me hint ?
>
> ->i_ino is long. It can't hold i_pos fully on 32bit arch, so we can't
> use ->i_no to store i_pos, and changing ->i_ino is unnecessary. If
> getattr() returned i_pos as ino, nobody see ->i_ino anymore except
> internal of kernel.
>
> Furthermore I think there is no issue even if server and client didn't
> have same ino. Because client just uses FH (nfs4 seems to be using
> stat.ino though).
Okay, I will share the result after checking and testing more(nfsv3 and nfsv4).
Thanks a lot!
> --
> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
>

Re: [PATCH v3 2/5] fat: allocate persistent inode numbers

[J. Bruce Fields]

On Mon, Sep 24, 2012 at 09:32:00PM +0900, OGAWA Hirofumi wrote:
> Namjae Jeon <linkinjeon@gmail.com> writes:
> 
> > 2012/9/24, OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>:
> >> Namjae Jeon <linkinjeon@gmail.com> writes:
> >>
> >>>> I see. fileid seems to be stat.ino on nfsd4. inode->i_ino is actually
> >>>> just a hash key of inode hash (exception is only in audit, iirc).
> >>>>
> >>>> So, what happens if we set "stat->ino = i_pos" on fat_getattr().
> >>>>
> >>>> int fat_getattr(struct vfsmount *mnt, struct dentry *dentry, struct
> >>>> kstat
> >>>> *stat)
> >>>> {
> >>>> 	struct inode *inode = dentry->d_inode;
> >>>> 	generic_fillattr(inode, stat);
> >>>> 	stat->blksize = MSDOS_SB(inode->i_sb)->cluster_size;
> >>>> 	if (opts->nfs == FAT_NFS_LIMITED) {
> >>>> 		/* Use i_pos for ino. This is used as fileid of nfs. */
> >>>> 		stat->ino = MSDOS_SB(inode->i_sb)->i_pos;
> >>
> >> 		stat->ino = fat_i_pos_read(MSDOS_SB(inode->i_sb), inode);
> >>
> >> Ouch, I forgot to use fat_i_pos_read().
> >>
> > There is some unclear thing.
> > When I see first mail, I think maybe you don't want to use i_pos for inode->ino.
> > FAT allocate inode->ino from i_unique on server side and If NFS client
> > use i_pos for inode->ino in fat_get_attr, inode numbers on each
> > client/server will still be mismatched.
> >
> > Would you plz give me hint ?
> 
> ->i_ino is long. It can't hold i_pos fully on 32bit arch, so we can't
> use ->i_no to store i_pos, and changing ->i_ino is unnecessary. If
> getattr() returned i_pos as ino, nobody see ->i_ino anymore except
> internal of kernel.

The NFS server must always return the same inode number for the same
filehandle.  To do otherwise is a bug.

> Furthermore I think there is no issue even if server and client didn't
> have same ino. Because client just uses FH (nfs4 seems to be using
> stat.ino though).

The client may expose a different inode number to userspace, but it's
probably the server-provided inode number that it's checking.

(And even if the Linux client didn't currently happen to do that check,
this would still be a bug.)

--b.

Re: [PATCH v3 2/5] fat: allocate persistent inode numbers

[OGAWA Hirofumi]

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

>> > There is some unclear thing.
>> > When I see first mail, I think maybe you don't want to use i_pos for inode->ino.
>> > FAT allocate inode->ino from i_unique on server side and If NFS client
>> > use i_pos for inode->ino in fat_get_attr, inode numbers on each
>> > client/server will still be mismatched.
>> >
>> > Would you plz give me hint ?
>> 
>> ->i_ino is long. It can't hold i_pos fully on 32bit arch, so we can't
>> use ->i_no to store i_pos, and changing ->i_ino is unnecessary. If
>> getattr() returned i_pos as ino, nobody see ->i_ino anymore except
>> internal of kernel.
>
> The NFS server must always return the same inode number for the same
> filehandle.  To do otherwise is a bug.
>
>> Furthermore I think there is no issue even if server and client didn't
>> have same ino. Because client just uses FH (nfs4 seems to be using
>> stat.ino though).
>
> The client may expose a different inode number to userspace, but it's
> probably the server-provided inode number that it's checking.
>
> (And even if the Linux client didn't currently happen to do that check,
> this would still be a bug.)

In this context, inode number != inode->i_ino, right? It should be
kstat.ino, and in FAT case, it will return i_pos always. Otherwise 64bit
inode number would not work.

So, I think we are doing right thing for now.

Anyway, thanks for your review.
-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH v3 2/5] fat: allocate persistent inode numbers

[J. Bruce Fields]

On Tue, Sep 25, 2012 at 01:16:45AM +0900, OGAWA Hirofumi wrote:
> "J. Bruce Fields" <bfields@fieldses.org> writes:
> 
> >> > There is some unclear thing.
> >> > When I see first mail, I think maybe you don't want to use i_pos for inode->ino.
> >> > FAT allocate inode->ino from i_unique on server side and If NFS client
> >> > use i_pos for inode->ino in fat_get_attr, inode numbers on each
> >> > client/server will still be mismatched.
> >> >
> >> > Would you plz give me hint ?
> >> 
> >> ->i_ino is long. It can't hold i_pos fully on 32bit arch, so we can't
> >> use ->i_no to store i_pos, and changing ->i_ino is unnecessary. If
> >> getattr() returned i_pos as ino, nobody see ->i_ino anymore except
> >> internal of kernel.
> >
> > The NFS server must always return the same inode number for the same
> > filehandle.  To do otherwise is a bug.
> >
> >> Furthermore I think there is no issue even if server and client didn't
> >> have same ino. Because client just uses FH (nfs4 seems to be using
> >> stat.ino though).
> >
> > The client may expose a different inode number to userspace, but it's
> > probably the server-provided inode number that it's checking.
> >
> > (And even if the Linux client didn't currently happen to do that check,
> > this would still be a bug.)
> 
> In this context, inode number != inode->i_ino, right? It should be
> kstat.ino, and in FAT case, it will return i_pos always. Otherwise 64bit
> inode number would not work.
> 
> So, I think we are doing right thing for now.

Oh, OK.  On a quick check, yes, the numbers the server returns to
clients are taken from either kstat.ino or the ino argument of the
filldir function.

--b.

Re: [PATCH v3 2/5] fat: allocate persistent inode numbers

[Namjae Jeon]

Hi OGAWA.

It works fine. there is no estale error while memory reclaim.
I will make patchset again as review comment and your suggestion
(encode_fh, fat_getattr).

Thanks!

2012/9/25, J. Bruce Fields <bfields@fieldses.org>:
> On Tue, Sep 25, 2012 at 01:16:45AM +0900, OGAWA Hirofumi wrote:
>> "J. Bruce Fields" <bfields@fieldses.org> writes:
>>
>> >> > There is some unclear thing.
>> >> > When I see first mail, I think maybe you don't want to use i_pos for
>> >> > inode->ino.
>> >> > FAT allocate inode->ino from i_unique on server side and If NFS
>> >> > client
>> >> > use i_pos for inode->ino in fat_get_attr, inode numbers on each
>> >> > client/server will still be mismatched.
>> >> >
>> >> > Would you plz give me hint ?
>> >>
>> >> ->i_ino is long. It can't hold i_pos fully on 32bit arch, so we can't
>> >> use ->i_no to store i_pos, and changing ->i_ino is unnecessary. If
>> >> getattr() returned i_pos as ino, nobody see ->i_ino anymore except
>> >> internal of kernel.
>> >
>> > The NFS server must always return the same inode number for the same
>> > filehandle.  To do otherwise is a bug.
>> >
>> >> Furthermore I think there is no issue even if server and client didn't
>> >> have same ino. Because client just uses FH (nfs4 seems to be using
>> >> stat.ino though).
>> >
>> > The client may expose a different inode number to userspace, but it's
>> > probably the server-provided inode number that it's checking.
>> >
>> > (And even if the Linux client didn't currently happen to do that check,
>> > this would still be a bug.)
>>
>> In this context, inode number != inode->i_ino, right? It should be
>> kstat.ino, and in FAT case, it will return i_pos always. Otherwise 64bit
>> inode number would not work.
>>
>> So, I think we are doing right thing for now.
>
> Oh, OK.  On a quick check, yes, the numbers the server returns to
> clients are taken from either kstat.ino or the ino argument of the
> filldir function.
>
> --b.
>