Re: [PATCH take 3][RFC] fat: Save FAT root directory timestamps to volume label

[OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>]

Jorg Schummer <ext-jorg.2.schummer@nokia.com> writes:

> Standard FAT implementations cannot store any of the FAT root directory's
> timestamps. This commit adds the mount option 'rootts', which allows saving
> the FAT root directory timestamps as the timestamps of the FAT volume label
> directory entry. At least Mac OS X is known to support the same mechanism
> and interoperate with this commit.
>
> When mounting, the following values can be specified for the 'rootts' mount
> option:
>
>   "rootts=ignore"   ignores root directory timestamps. All timestamps are
>                     reset to 0 (1/1/1970). This has been the FAT behaviour
>                     prior to this patch.
>
>   "rootts=preserve" tries to load and save the root directory's timestamps
>                     if a volume label entry exists. The mtime and atime are
>                     corrected based on root directory entries' ctime. This
>                     is the default.
>
>   "rootts=save"     tries to load and save the root directory's timestamps.
>                     The mtime and atime are corrected based on root
>                     directory entries' ctime. If the root directory was
>                     accessed but no volume label entry exists, the label
>                     "NO NAME" is created.

Um..., I'm still not sure though. rootts=save may not be needed for
this, because it seems a bit complex, and it can do on userland...

And some quick review of implement..

> +	if (inode->i_ino == MSDOS_ROOT_INO) {
> +		if (sbi->options.root_ts == FAT_ROOT_TS_IGNORE)
> +			return 0;
> +
> +		/* Write the timestamps of a FAT file system's root directory
> +		 * as the timestamps of the file system's label dir entry. */
> +
> +		if (!MSDOS_I(inode)->i_pos) {
> +			struct buffer_head *bh;
> +			struct msdos_dir_entry *de;
> +			loff_t i_pos;
> +
> +			if (sbi->options.root_ts != FAT_ROOT_TS_SAVE)
> +				/* No label present, but we should not
> +				   create one. Thus not complaining. */
> +				return 0;
> +
> +			printk(KERN_INFO "FAT: creating volume label "
> +			       "on %s to save root dir timestamps\n",
> +			       sb->s_id);
> +
> +			err = fat_add_label_entry(inode, NULL);
> +			if (err)
> +				return err;
> +			err = fat_get_label_entry(inode, &bh, &de, &i_pos);
> +			if (err)
> +				return err;

In this path, we don't have inode->i_mutex, so to look directry up is
wrong. I think adding label may move to userland.

I guess we should do all of preparation in fill_super().

> +			remove_inode_hash(inode);
> +			fat_attach(inode, i_pos);
> +			insert_inode_hash(inode);
> +			brelse(bh);

Root inode should never change the position, so this shouldn't be
needed.

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