Date:	Sat, 13 Oct 2012 23:02:09 +0900
From:	Namjae Jeon <linkinjeon@...il.com>
To:	OGAWA Hirofumi <hirofumi@...l.parknet.co.jp>
Cc:	akpm@...ux-foundation.org, bfields@...ldses.org,
	viro@...iv.linux.org.uk, linux-kernel@...r.kernel.org,
	Namjae Jeon <namjae.jeon@...sung.com>,
	Ravishankar N <ravi.n1@...sung.com>,
	Amit Sahrawat <a.sahrawat@...sung.com>
Subject: Re: [PATCH v4 2/4] fat (exportfs): rebuild inode if ilookup() fails

Hi. OGAWA.
2012/10/13 OGAWA Hirofumi <hirofumi@...l.parknet.co.jp>:
> Namjae Jeon <linkinjeon@...il.com> writes:
>
>> --- a/fs/fat/fat.h
>> +++ b/fs/fat/fat.h
>> @@ -214,6 +214,13 @@ static inline sector_t fat_clus_to_blknr(struct msdos_sb_info *sbi, int clus)
>>               + sbi->data_start;
>>  }
>>
>> +static inline void fat_get_blknr_offset(struct msdos_sb_info *sbi,
>> +                             loff_t i_pos, sector_t *blknr, int *offset)
>> +{
>> +     *blknr = i_pos >> sbi->dir_per_block_bits;
>> +     *offset = i_pos & (sbi->dir_per_block - 1);
>> +}
>> +
>
> Let's separate fat_get_blknr_offset() cleanup and others.
Okay.
>
>> +extern loff_t fat_i_pos_read(struct msdos_sb_info *sbi, struct inode *inode);
>
> [...]
>
>> -static inline loff_t fat_i_pos_read(struct msdos_sb_info *sbi,
>> +loff_t fat_i_pos_read(struct msdos_sb_info *sbi,
>>                                   struct inode *inode)
>>  {
>>       loff_t i_pos;
>
> Let's move fat_i_pos_read() to fat.h to make consists with
> fat_get_blknr_offset().
Okay.
>
>>  static const struct export_operations fat_export_ops = {
>> +     .encode_fh      = fat_encode_fh,
>>       .fh_to_dentry   = fat_fh_to_dentry,
>>       .fh_to_parent   = fat_fh_to_parent,
>>       .get_parent     = fat_get_parent,
>> diff --git a/fs/fat/nfs.c b/fs/fat/nfs.c
>> index ef4b5fa..156903b 100644
>> --- a/fs/fat/nfs.c
>> +++ b/fs/fat/nfs.c
>> @@ -14,6 +14,14 @@
>>  #include <linux/exportfs.h>
>>  #include "fat.h"
>>
>> +struct fat_fid {
>> +     u32 ino;
>> +     u32 gen;
>> +     u64 i_pos;
>> +     u32 parent_ino;
>> +     u32 parent_gen;
>> +} __packed;
>
> This is sizeof(fat_fid)/sizoef(u32) == 6. IIRC, nfsv2 is not supporting
> FH > 6, true?
I will check and share the result.
>
>> +int
>> +fat_encode_fh(struct inode *inode, __u32 *fh, int *lenp, struct inode *parent)
>> +{
>> +     int len = *lenp;
>> +     struct msdos_sb_info *sbi = MSDOS_SB(inode->i_sb);
>> +     struct fat_fid *fid = (struct fat_fid *) fh;
>> +     loff_t i_pos;
>> +     int type = FILEID_INO32_GEN;
>> +
>> +     if (parent && (len < 5)) {
>> +             *lenp = 5;
>> +             return 255;
>> +     } else if (len < 3) {
>> +             *lenp = 3;
>> +             return 255;
>> +     }
>> +
>> +     i_pos = fat_i_pos_read(sbi, inode);
>> +     *lenp = 3;
>> +     fid->ino = inode->i_ino;
>> +     fid->gen = inode->i_generation;
>> +     fid->i_pos = i_pos;
>> +     if (parent) {
>> +             fid->parent_ino = parent->i_ino;
>> +             fid->parent_gen = parent->i_generation;
>> +             type = FILEID_INO32_GEN_PARENT;
>> +             *lenp = 5;
>> +     }
>> +
>> +     return type;
>> +}
>
> I was also thinking to use same FH though, because of limitation of FH
> size. It looks like to be better to separate with "stale_rw".
>
> So, how about to separate at export_operations level?
Good idea!
>
> I.e. (move export_operations to fat/nfs.c)
>
> For stale_rw,
>
> const struct export_operations fat_export_ops = {
>         .fh_to_dentry   = fat_fh_to_dentry,
>         .fh_to_parent   = fat_fh_to_parent,
>         .get_parent     = fat_get_parent,
> }
>
> For nostale_ro,
>
> const struct export_operations fat_export_ops = {
>         .fh_to_dentry   = fat_encode_fh,
>         .fh_to_dentry   = fat_fh_to_dentry,
>         .fh_to_parent   = fat_fh_to_parent,
>         .get_parent     = fat_get_parent,
> }
>
> And we have to think about FH format. I guess we don't need to
> inode->i_ino for nostale_ro.
>
> Maximum i_pos is 40bits, and i_gen is 32bit. So, inode and parent inode
> fits to FH of 5 len, we have to pack those though.
>
> I.e.
>
> struct fat_fid {
>         u32 i_gen;
>         u32 i_pos_low;
>         u16 i_pos_hi;
>         u16 parent_i_pos_hi;
>         u32 parent_i_pos_low;
>         u32 parent_i_gen;
> } __packed;
>
> And define FILEID_FAT_WITHOUT_PARENT and FILEID_FAT_WITH_PARENT.
>
> User of i_ino is only ilookup, right? So, we can add fat_ilookup() for it.
Okay.
>
> struct inode *fat_ilookup(struct super_block *sb, u64 ino)
> {
>         if (stale_rw)
>                 return ilookup(sb, ino);
>
>         return fat_iget(sb, i_pos);
> }
>
> And I noticed we need lock for fat_build_inode() for nostale_ro. Because
> fat_nfs_get_inode() doesn't hold i_mutex of parent dir, right? So, add
> lock to fat_build_inode()
>
> static inline void fat_build_inode_lock(struct msdos_sb_info *sbi)
> {
>         if (sbi->options.nfs == FAT_NFS_NOSTALE_RO)
>                 mutex_lock(&sbi->nfs_build_inode_lock);
> }
>
> static inline void fat_build_inode_unlock(struct msdos_sb_info *sbi)
> {
>         if (sbi->options.nfs == FAT_NFS_NOSTALE_RO)
>                 mutex_unlock(&sbi->nfs_build_inode_lock);
> }
Thanks for specific review and suggestion. :)
I will change the patches as your suggestion.
Hum.. And currently these patch-set was pushed to -mm tree and linux-next.
I am confusing I should request to revert these patch-set or make new
patches base on these patch-set....

Thanks.
> --
> OGAWA Hirofumi <hirofumi@...l.parknet.co.jp>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists