adfs: add hexadecimal filetype suffix option

From: Stuart Swales<[email protected]>

ADFS (FileCore) storage complies with the RISC OS filetype specification (12 bits of file type information is stored in the file load address, rather than using a file extension). The existing driver largely ignores this information and does not present it to the end user.

It is desirable that stored filetypes be made visible to the end user to facilitate a precise copy of data and metadata from a hard disc (or image thereof) into a RISC OS emulator (such as RPCEmu) or to a network share which can be accessed by real Acorn systems.

This patch implements a per-mount filetype suffix option (use -o ftsuffix=1) to present any filetype as a ,xyz hexadecimal suffix on each file. This type suffix is compatible with that used by RISC OS systems that access network servers using NFS client software and by RPCemu's host filing system.

...

(quoted diff) +2 -0

+/* RISC OS 12-bit filetype converts to ,xyz hex filename suffix */
+static inline char lowercase_hex_digit(__u16 i)

struct adfs_dir_ops {
int (*read)(struct super_block *sb, unsigned int id, unsigned int sz, struct adfs_dir *dir);
int (*setpos)(struct adfs_dir *dir, unsigned int fpos);
diff -uprN -X linux-2.6.37-vanilla/Documentation/dontdiff linux-2.6.37-vanilla/fs/adfs/dir_f.c linux-2.6.37-sks-adfs/fs/adfs/dir_f.c

fs/adfs/dir_f.c +0 -1

@@ -52,7 +52,6 @@ static inline int adfs_readname(char *bu
  			*buf++ = *ptr;
  		ptr++;
  	}
-	*buf = '\0';
 
  	return buf - old_buf;
  }

fs/adfs/dir_f.c +7 -0

+		/* optionally append the ,xyz hex filetype suffix */
+		if (ADFS_SB(dir->sb)->s_ftsuffix)
+			obj->name_len +=
+				append_filetype_suffix(
+					&obj->name[obj->name_len],
+					obj->filetype);
+	}
  }
 
  /*
 
...

@@ -447,11 +456,13 @@ static int adfs_fill_super(struct super_
 
  	root_obj.parent_id = root_obj.file_id = le32_to_cpu(dr->root);

fs/adfs/dir_f.c +0 -0

  	/*
  	 * If this is a F+ disk with variable length directories,
 
...

Quoted 12 lines · On Wed, 12 Jan 2011 18:07:23 +0000 Stuart Swales…

From: Stuart Swales<[email protected]>

ADFS (FileCore) storage complies with the RISC OS filetype specification (12 bits of file type information is stored in the file load address, rather than using a file extension). The existing driver largely ignores this information and does not present it to the end user.

It is desirable that stored filetypes be made visible to the end user to facilitate a precise copy of data and metadata from a hard disc (or image thereof) into a RISC OS emulator (such as RPCEmu) or to a network share which can be accessed by real Acorn systems.

Quoted 5 lines

This patch implements a per-mount filetype suffix option (use -o ftsuffix=1) to present any filetype as a ,xyz hexadecimal suffix on each file. This type suffix is compatible with that used by RISC OS systems that access network servers using NFS client software and by RPCemu's host filing system.

I don't really understand this. Are you saying that if a file on an adfs partition has name "foo", and one does a mount -o ftsuffix=1' then an ls' would show "foo,ABC"?

If so, that sounds strange but I assume you know what you're doing.

Was `mount -o remount,ftsuffix=1' implemented and tested?

Am I correct in assuming that "mount -o remount,ftsuffix=0" turns this off again?

I wonder what are the implications of changing filenames on the fly with a remount.

We have a Documentation/filesystems/adfs.txt. Please update it.

Again, the patch was mangled by your email client in some fashion and generates 100% rejects.

Please pass the diff through scripts/checkpatch.pl and address and warnings which you do not disagree with.

[PATCH] adfs: add hexadecimal filetype suffix option

ADFS (FileCore) storage complies with the RISC OS filetype specification (12 bits of file type information is stored in the file load address, rather than using a file extension). The existing driver largely ignores this information and does not present it to the end user.

It is desirable that stored filetypes be made visible to the end user to facilitate a precise copy of data and metadata from a hard disc (or image thereof) into a RISC OS emulator (such as RPCEmu) or to a network share which can be accessed by real Acorn systems.

This patch implements a per-mount filetype suffix option (use -o ftsuffix=1) to present any filetype as a ,xyz hexadecimal suffix on each file. This type suffix is compatible with that used by RISC OS systems that access network servers using NFS client software and by RPCemu's host filing system.

Signed-off-by Stuart Swales <[email protected]>

Quoted 13 lines · On Friday 21 January 2011 15:34:12 Stuart Swales wrote:

[PATCH] adfs: add hexadecimal filetype suffix option

ADFS (FileCore) storage complies with the RISC OS filetype specification (12 bits of file type information is stored in the file load address, rather than using a file extension). The existing driver largely ignores this information and does not present it to the end user.

It is desirable that stored filetypes be made visible to the end user to facilitate a precise copy of data and metadata from a hard disc (or image thereof) into a RISC OS emulator (such as RPCEmu) or to a network share which can be accessed by real Acorn systems.

Quoted 6 lines

This patch implements a per-mount filetype suffix option (use -o ftsuffix=1) to present any filetype as a ,xyz hexadecimal suffix on each file. This type suffix is compatible with that used by RISC OS systems that access network servers using NFS client software and by RPCemu's host filing system.

Signed-off-by Stuart Swales <[email protected]>

The patch looks fine to me, but it tells me that you have some knowledge and interest in this file system. Adfs is currently one of only a handful of modules in the kernel that still uses the big kernel lock, because nobody so far had enough motivation to fix this.

Would you be able to take a look at this? The straightforward approach would be to add a mutex to adfs_sb_info and use that in place of lock_kernel. It's mostly a matter of testing to make sure that no deadlocks get introduced in the process.

Quoted 1 line

Was `mount -o remount,ftsuffix=1' implemented and tested?

I didn't specifically check it out with remount. Is that likely to be a problem?

Quoted 2 lines

Am I correct in assuming that "mount -o remount,ftsuffix=0" turns this off again?

Quoted 4 lines · Arnd Bergmann

Would you be able to take a look at this? The straightforward approach would be to add a mutex to adfs_sb_info and use that in place of lock_kernel. It's mostly a matter of testing to make sure that no deadlocks get introduced in the process.

Note that write support is very limited. From memory, the only writing it supports is changing directory entries and writing data into existing files (but not extending or truncating them.)

The only thing that's supported is updating of existing directory entries, for which it has a read/write lock to protect against multiple callers. I think I assumed at the time that I wouldn't rely on the BKL, so it should be safe to kill it.

Quoted 7 lines · On Friday 21 January 2011 18:26:58 Russell King wrote:

Quoted 4 lines · Arnd Bergmann

Would you be able to take a look at this? The straightforward approach would be to add a mutex to adfs_sb_info and use that in place of lock_kernel. It's mostly a matter of testing to make sure that no deadlocks get introduced in the process.

Quoted 8 lines

Note that write support is very limited. From memory, the only writing it supports is changing directory entries and writing data into existing files (but not extending or truncating them.)

The only thing that's supported is updating of existing directory entries, for which it has a read/write lock to protect against multiple callers. I think I assumed at the time that I wouldn't rely on the BKL, so it should be safe to kill it.

Makes sense. Proving that the BKL is not needed is obviously preferred over replacing it with a mutex. After your explanation and another look at the code, I agree that we can simply remove all references to the BKL from adfs, unless someone can find a reason we still need it.

Stuart, could you add this patch to your series then, and give it some testing?

Arnd

8<-----
Subject: adfs: remove the big kernel lock

According to Russell King, adfs was written to not require the big kernel lock, and all inode updates are done under adfs_dir_lock.

All other metadata in adfs is read-only and does not require locking. The use of the BKL is the result of various pushdowns from the VFS operations.

Signed-off-by Arnd Bergmann<[email protected]>

Cc Russell King<[email protected]>

Cc Stuart Swales<[email protected]>

---

  fs/adfs/dir.c   |    6 ------
  fs/adfs/inode.c |    6 ------
  fs/adfs/super.c |   13 +------------
  3 files changed, 1 insertions(+), 24 deletions(-)

fs/adfs/dir.c +0 -6

index 3b4a764..3d83075 100644
@@ -9,7 +9,6 @@
   *
   *  Common directory handling for ADFS
   */
-#include<linux/smp_lock.h>
  #include "adfs.h"
 
  /*

@@ -27,8 +26,6 @@ adfs_readdir(struct file *filp, void *dirent, filldir_t filldir)
  	struct adfs_dir dir;
  	int ret = 0;
 
-	lock_kernel();	
-
  	if (filp->f_pos>>  32)
  		goto out;
 

@@ -70,7 +67,6 @@ free_out:
  	ops->free(&dir);
 
  out:
-	unlock_kernel();
  	return ret;
  }
 

@@ -276,7 +272,6 @@ adfs_lookup(struct inode *dir, struct dentry *dentry, struct nameidata *nd)
  	struct object_info obj;
  	int error;
 
-	lock_kernel();
  	error = adfs_dir_lookup_byname(dir,&dentry->d_name,&obj);
  	if (error == 0) {
  		error = -EACCES;

@@ -288,7 +283,6 @@ adfs_lookup(struct inode *dir, struct dentry *dentry, struct nameidata *nd)
  		if (inode)
  			error = 0;
  	}
-	unlock_kernel();
  	d_add(dentry, inode);
  	return ERR_PTR(error);
  }

fs/adfs/inode.c +0 -6

index 65794b8..09fe401 100644
@@ -7,7 +7,6 @@
   * it under the terms of the GNU General Public License version 2 as
   * published by the Free Software Foundation.
   */
-#include<linux/smp_lock.h>
  #include<linux/buffer_head.h>
  #include<linux/writeback.h>
  #include "adfs.h"

@@ -316,8 +315,6 @@ adfs_notify_change(struct dentry *dentry, struct iattr *attr)
  	unsigned int ia_valid = attr->ia_valid;
  	int error;
 
-	lock_kernel();
-
  	error = inode_change_ok(inode, attr);
 
  	/*

@@ -359,7 +356,6 @@ adfs_notify_change(struct dentry *dentry, struct iattr *attr)
  	if (ia_valid&  (ATTR_SIZE | ATTR_MTIME | ATTR_MODE))
  		mark_inode_dirty(inode);
  out:
-	unlock_kernel();
  	return error;
  }
 

@@ -374,7 +370,6 @@ int adfs_write_inode(struct inode *inode, struct writeback_control *wbc)
  	struct object_info obj;
  	int ret;
 
-	lock_kernel();
  	obj.file_id	= inode->i_ino;
  	obj.name_len	= 0;
  	obj.parent_id	= ADFS_I(inode)->parent_id;

@@ -384,6 +379,5 @@ int adfs_write_inode(struct inode *inode, struct writeback_control *wbc)
  	obj.size	= inode->i_size;
 
  	ret = adfs_dir_update(sb,&obj, wbc->sync_mode == WB_SYNC_ALL);
-	unlock_kernel();
  	return ret;
  }

fs/adfs/super.c +1 -12

index 2d79540..06d7388 100644
@@ -14,7 +14,6 @@
  #include<linux/mount.h>
  #include<linux/seq_file.h>
  #include<linux/slab.h>
-#include<linux/smp_lock.h>
  #include<linux/statfs.h>
  #include "adfs.h"
  #include "dir_f.h"

@@ -120,15 +119,11 @@ static void adfs_put_super(struct super_block *sb)
  	int i;
  	struct adfs_sb_info *asb = ADFS_SB(sb);
 
-	lock_kernel();
-
  	for (i = 0; i<  asb->s_map_size; i++)
  		brelse(asb->s_map[i].dm_bh);
  	kfree(asb->s_map);
  	kfree(asb);
  	sb->s_fs_info = NULL;
-
-	unlock_kernel();
  }
 
  static int adfs_show_options(struct seq_file *seq, struct vfsmount *mnt)

@@ -359,15 +354,11 @@ static int adfs_fill_super(struct super_block *sb, void *data, int silent)
  	struct adfs_sb_info *asb;
  	struct inode *root;
 
-	lock_kernel();
-
  	sb->s_flags |= MS_NODIRATIME;
 
  	asb = kzalloc(sizeof(*asb), GFP_KERNEL);
-	if (!asb) {
-		unlock_kernel();
+	if (!asb)
  		return -ENOMEM;
-	}
  	sb->s_fs_info = asb;
 
  	/* set default options */

@@ -485,7 +476,6 @@ static int adfs_fill_super(struct super_block *sb, void *data, int silent)
  		adfs_error(sb, "get root inode failed\n");
  		goto error;
  	}
-	unlock_kernel();
  	return 0;
 
  error_free_bh:

@@ -493,7 +483,6 @@ error_free_bh:
  error:
  	sb->s_fs_info = NULL;
  	kfree(asb);
-	unlock_kernel();
  	return -EINVAL;
  }
 

Review

Quoted 5 lines · On Wed, 12 Jan 2011 18:07:23 +0000 Stuart Swales…

From: Stuart Swales<[email protected]>

(quoted diff) +3 -0

+static inline int append_filetype_suffix(char *buf, __u16 filetype)
+{
+     if ((__u16) -1 == filetype)

unneeded cast.

Quoted 9 lines · Andrew Morton

Review

Quoted 5 lines · On Wed, 12 Jan 2011 18:07:23 +0000 Stuart Swales…

From: Stuart Swales<[email protected]>

(quoted diff) +3 -0

+static inline int append_filetype_suffix(char *buf, __u16 filetype)
+{
+ ?? ?? if ((__u16) -1 == filetype)

unneeded cast.

My compiler tends to disagree. On current mainline, it says:

fs/adfs/adfs.h: In function ???append_filetype_suffix???:
fs/adfs/adfs.h:115: warning: comparison is always false due to limited range of data type

Quoted 9 lines · Andrew Morton

Review

Quoted 5 lines · On Wed, 12 Jan 2011 18:07:23 +0000 Stuart Swales…

From: Stuart Swales<[email protected]>

(quoted diff) +3 -0

+static inline int append_filetype_suffix(char *buf, __u16 filetype)
+{
+ __ __ if ((__u16) -1 == filetype)

unneeded cast.

My compiler tends to disagree. On current mainline, it says:

fs/adfs/adfs.h: In function ___append_filetype_suffix___:
fs/adfs/adfs.h:115: warning: comparison is always false due to limited range of data type

Quoted 11 lines · On Wed, 23 Mar 2011 21:36:48 +0100 Geert Uytterhoeven<[email protected]>…

Quoted 9 lines · Andrew Morton

Review

Quoted 5 lines · On Wed, 12 Jan 2011 18:07:23 +0000 Stuart…

From: Stuart Swales<[email protected]>

(quoted diff) +3 -0

+static inline int append_filetype_suffix(char *buf, __u16 filetype)
+{
+ __ __ if ((__u16) -1 == filetype)

unneeded cast.

Quoted 6 lines

My compiler tends to disagree. On current mainline, it says:

fs/adfs/adfs.h: In function ___append_filetype_suffix___:
fs/adfs/adfs.h:115: warning: comparison is always false due to limited range of data type

hm, OK, I'll add a cast.

The code seems a bit fishy - it wants 0xffff but the comment says it's handling a 12-bit quantity. Obviously the all-ones pattern has some magical out-of-band meaning here, but it is not explained what that meaning *is*. Perhaps that is described elsewhere.