Re: [PATCH] adfs: add hexadecimal filetype suffix option

Re: [PATCH] adfs: add hexadecimal filetype suffix option

[Andrew Morton]

On Wed, 12 Jan 2011 18:07:23 +0000
Stuart Swales <stuart.swales.croftnuisk@gmail.com> wrote:

> From: Stuart Swales<stuart.swales.croftnuisk@gmail.com>
> 
> 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.
> 

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.

>
> ...
>
> +/* RISC OS 12-bit filetype converts to ,xyz hex filename suffix */
> +static inline char lowercase_hex_digit(__u16 i)
> +{
> +	if (i>= 10)
> +		return 'a' + (i - 10);
> +
> +	return '0' + i;
> +}
> +
> +static inline int append_filetype_suffix(char *buf, __u16 filetype)
> +{
> +	if ((__u16) -1 == filetype)

unneeded cast.

> +		return 0;
> +
> +	*buf++ = ',';
> +	*buf++ = lowercase_hex_digit((filetype>>  8)&  0x0f);
> +	*buf++ = lowercase_hex_digit((filetype>>  4)&  0x0f);
> +	*buf++ = lowercase_hex_digit((filetype>>  0)&  0x0f);
> +	return 4;
> +}

Kernel code tends to avoid using the `if (42 == foo)' construct.  gcc
will generate a warning if the programmer accidentally typed '=', and
it Just Looks Weird.

append_filetype_suffix() is too large to be inlined.

append_filetype_suffix() should use more of the general library
functions.  `grep hex include/linux/kernel.h'.  hex2bin() *should* be
directly applicable here but it isn't because it's stupid.


>   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
> --- linux-2.6.37-vanilla/fs/adfs/dir_f.c	2011-01-05 00:50:19.000000000 +0000
> +++ linux-2.6.37-sks-adfs/fs/adfs/dir_f.c	2011-01-12 17:01:49.000000000 +0000
> @@ -52,7 +52,6 @@ static inline int adfs_readname(char *bu
>   			*buf++ = *ptr;
>   		ptr++;
>   	}
> -	*buf = '\0';
> 
>   	return buf - old_buf;
>   }
> @@ -208,7 +207,10 @@ release_buffers:
>    * convert a disk-based directory entry to a Linux ADFS directory entry
>    */
>   static inline void
> -adfs_dir2obj(struct object_info *obj, struct adfs_direntry *de)
> +adfs_dir2obj(
> +	struct adfs_dir *dir,
> +	struct object_info *obj,
> +	struct adfs_direntry *de)

The xfs guys lay out their functions like that, but they're in their
own little world.

>   {
>   	obj->name_len =	adfs_readname(obj->name, de->dirobname, ADFS_F_NAME_LEN);
>   	obj->file_id  = adfs_readval(de->dirinddiscadd, 3);
> @@ -216,6 +218,22 @@ adfs_dir2obj(struct object_info *obj, st
>   	obj->execaddr = adfs_readval(de->direxec, 4);
>   	obj->size     = adfs_readval(de->dirlen,  4);
>   	obj->attr     = de->newdiratts;
> +
> +	obj->filetype = (__u16) -1;
> +	/* object is a file and is filetyped and timestamped?
> +	 * RISC OS 12-bit filetype is stored in load_address[19:8]
> +	 */

Multiline comments are typically laid out as


	/*
	 * object is a file and is filetyped and timestamped?
	 * RISC OS 12-bit filetype is stored in load_address[19:8]
	 */

> +	if ((0 == (obj->attr&  ADFS_NDA_DIRECTORY))&&
> +		(0xfff00000 == (0xfff00000&  obj->loadaddr))) {
> +		obj->filetype = (__u16) ((0x000fff00&  obj->loadaddr)>>  8);

Much coding-style grief there.

> +		/* 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);
>   	root_obj.name_len  = 0;
> -	root_obj.loadaddr  = 0;
> -	root_obj.execaddr  = 0;
> +	/* Set root object date as 01 Jan 1987 00:00:00 */
> +	root_obj.loadaddr  = 0xfff0003f;
> +	root_obj.execaddr  = 0xec22c000;
>   	root_obj.size	   = ADFS_NEWDIR_SIZE;
>   	root_obj.attr	   = ADFS_NDA_DIRECTORY   | ADFS_NDA_OWNER_READ |
>   			     ADFS_NDA_OWNER_WRITE | ADFS_NDA_PUBLIC_READ;
> +	root_obj.filetype  = (__u16) -1;

unneeded cast.

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

[PATCH] adfs: add hexadecimal filetype suffix option

[Stuart Swales]

[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 <stuart.swales.croftnuisk@gmail.com>

---

Bugzilla # 26492
(ADFS filesystem filetype metadata lost)

Test files are attached in Bugzilla issue.

NB. Updated patch with style fixes as suggested by Andrew.


diff -X linux-2.6.37-vanilla/Documentation/dontdiff -uprN linux-2.6.37-vanilla/Documentation/filesystems/adfs.txt linux-2.6.37-sks-adfs/Documentation/filesystems/adfs.txt
--- linux-2.6.37-vanilla/Documentation/filesystems/adfs.txt	2011-01-05 00:50:19.000000000 +0000
+++ linux-2.6.37-sks-adfs/Documentation/filesystems/adfs.txt	2011-01-21 14:10:27.000000000 +0000
@@ -9,6 +9,9 @@ Mount options for ADFS
 		will be nnn.  Default 0700.
   othmask=nnn	The permission mask for ADFS 'other' permissions
 		will be nnn.  Default 0077.
+  ftsuffix=n	When ftsuffix=0, no file type suffix will be applied.
+		When ftsuffix=1, a hexadecimal suffix corresponding to
+		the RISC OS file type will be added.  Default 0.

 Mapping of ADFS permissions to Linux permissions
 ------------------------------------------------
@@ -55,3 +58,18 @@ Mapping of ADFS permissions to Linux per

   You can therefore tailor the permission translation to whatever you
   desire the permissions should be under Linux.
+
+RISC OS file type suffix
+------------------------
+
+  RISC OS file types are stored in bits 19..8 of the file load address.
+
+  To enable non-RISC OS systems to be used to store files without losing
+  file type information, a file naming convention was devised (initially
+  for use with NFS) such that a hexadecimal suffix of the form ,xyz
+  denoted the file type: e.g. BasicFile,ffb is a BASIC (0xffb) file.  This
+  naming convention is now also used by RISC OS emulators such as RPCEmu.
+
+  Mounting an ADFS disc with option ftsuffix=1 will cause appropriate file
+  type suffixes to be appended to file names read from a directory.  If the
+  ftsuffix option is zero or omitted, no file type suffixes will be added.
diff -X linux-2.6.37-vanilla/Documentation/dontdiff -uprN linux-2.6.37-vanilla/fs/adfs/adfs.h linux-2.6.37-sks-adfs/fs/adfs/adfs.h
--- linux-2.6.37-vanilla/fs/adfs/adfs.h	2011-01-05 00:50:19.000000000 +0000
+++ linux-2.6.37-sks-adfs/fs/adfs/adfs.h	2011-01-21 13:50:12.000000000 +0000
@@ -50,6 +50,7 @@ struct adfs_sb_info {
 	gid_t		s_gid;		/* owner gid				 */
 	umode_t		s_owner_mask;	/* ADFS owner perm -> unix perm		 */
 	umode_t		s_other_mask;	/* ADFS other perm -> unix perm		 */
+	int		s_ftsuffix;	/* ,xyz hex filetype suffix option */

 	__u32		s_ids_per_zone;	/* max. no ids in one zone		 */
 	__u32		s_idlen;	/* length of ID in map			 */
@@ -89,7 +90,7 @@ struct adfs_dir {
 /*
  * This is the overall maximum name length
  */
-#define ADFS_MAX_NAME_LEN	256
+#define ADFS_MAX_NAME_LEN	(256 + 4) /* +4 for ,xyz hex filetype suffix */
 struct object_info {
 	__u32		parent_id;		/* parent object id	*/
 	__u32		file_id;		/* object id		*/
@@ -97,10 +98,26 @@ struct object_info {
 	__u32		execaddr;		/* execution address	*/
 	__u32		size;			/* size			*/
 	__u8		attr;			/* RISC OS attributes	*/
-	unsigned char	name_len;		/* name length		*/
+	unsigned int	name_len;		/* name length		*/
 	char		name[ADFS_MAX_NAME_LEN];/* file name		*/
+
+	/* RISC OS file type (12-bit: derived from loadaddr) */
+	__u16		filetype;
 };

+/* RISC OS 12-bit filetype converts to ,xyz hex filename suffix */
+static inline int append_filetype_suffix(char *buf, __u16 filetype)
+{
+	if (filetype == -1)
+		return 0;
+
+	*buf++ = ',';
+	*buf++ = hex_asc_lo(filetype >> 8);
+	*buf++ = hex_asc_lo(filetype >> 4);
+	*buf++ = hex_asc_lo(filetype >> 0);
+	return 4;
+}
+
 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 -X linux-2.6.37-vanilla/Documentation/dontdiff -uprN linux-2.6.37-vanilla/fs/adfs/dir_f.c linux-2.6.37-sks-adfs/fs/adfs/dir_f.c
--- linux-2.6.37-vanilla/fs/adfs/dir_f.c	2011-01-05 00:50:19.000000000 +0000
+++ linux-2.6.37-sks-adfs/fs/adfs/dir_f.c	2011-01-21 13:52:57.000000000 +0000
@@ -52,7 +52,6 @@ static inline int adfs_readname(char *bu
 			*buf++ = *ptr;
 		ptr++;
 	}
-	*buf = '\0';

 	return buf - old_buf;
 }
@@ -208,7 +207,8 @@ release_buffers:
  * convert a disk-based directory entry to a Linux ADFS directory entry
  */
 static inline void
-adfs_dir2obj(struct object_info *obj, struct adfs_direntry *de)
+adfs_dir2obj(struct adfs_dir *dir, struct object_info *obj,
+	struct adfs_direntry *de)
 {
 	obj->name_len =	adfs_readname(obj->name, de->dirobname, ADFS_F_NAME_LEN);
 	obj->file_id  = adfs_readval(de->dirinddiscadd, 3);
@@ -216,6 +216,23 @@ adfs_dir2obj(struct object_info *obj, st
 	obj->execaddr = adfs_readval(de->direxec, 4);
 	obj->size     = adfs_readval(de->dirlen,  4);
 	obj->attr     = de->newdiratts;
+	obj->filetype = -1;
+
+	/*
+	 * object is a file and is filetyped and timestamped?
+	 * RISC OS 12-bit filetype is stored in load_address[19:8]
+	 */
+	if ((0 == (obj->attr & ADFS_NDA_DIRECTORY)) &&
+		(0xfff00000 == (0xfff00000 & obj->loadaddr))) {
+		obj->filetype = (__u16) ((0x000fff00 & obj->loadaddr) >> 8);
+
+		/* 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);
+	}
 }

 /*
@@ -260,7 +277,7 @@ __adfs_dir_get(struct adfs_dir *dir, int
 	if (!de.dirobname[0])
 		return -ENOENT;

-	adfs_dir2obj(obj, &de);
+	adfs_dir2obj(dir, obj, &de);

 	return 0;
 }
diff -X linux-2.6.37-vanilla/Documentation/dontdiff -uprN linux-2.6.37-vanilla/fs/adfs/dir_fplus.c linux-2.6.37-sks-adfs/fs/adfs/dir_fplus.c
--- linux-2.6.37-vanilla/fs/adfs/dir_fplus.c	2011-01-05 00:50:19.000000000 +0000
+++ linux-2.6.37-sks-adfs/fs/adfs/dir_fplus.c	2011-01-21 13:53:43.000000000 +0000
@@ -147,6 +147,24 @@ adfs_fplus_getnext(struct adfs_dir *dir,
 		if (obj->name[i] == '/')
 			obj->name[i] = '.';

+	obj->filetype = -1;
+
+	/*
+	 * object is a file and is filetyped and timestamped?
+	 * RISC OS 12-bit filetype is stored in load_address[19:8]
+	 */
+	if ((0 == (obj->attr & ADFS_NDA_DIRECTORY)) &&
+		(0xfff00000 == (0xfff00000 & obj->loadaddr))) {
+		obj->filetype = (__u16) ((0x000fff00 & obj->loadaddr) >> 8);
+
+		/* 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);
+	}
+
 	dir->pos += 1;
 	ret = 0;
 out:
diff -X linux-2.6.37-vanilla/Documentation/dontdiff -uprN linux-2.6.37-vanilla/fs/adfs/inode.c linux-2.6.37-sks-adfs/fs/adfs/inode.c
--- linux-2.6.37-vanilla/fs/adfs/inode.c	2011-01-05 00:50:19.000000000 +0000
+++ linux-2.6.37-sks-adfs/fs/adfs/inode.c	2011-01-21 13:48:38.000000000 +0000
@@ -79,26 +79,13 @@ static const struct address_space_operat
 	.bmap		= _adfs_bmap
 };

-static inline unsigned int
-adfs_filetype(struct inode *inode)
-{
-	unsigned int type;
-
-	if (ADFS_I(inode)->stamped)
-		type = (ADFS_I(inode)->loadaddr >> 8) & 0xfff;
-	else
-		type = (unsigned int) -1;
-
-	return type;
-}
-
 /*
  * Convert ADFS attributes and filetype to Linux permission.
  */
 static umode_t
 adfs_atts2mode(struct super_block *sb, struct inode *inode)
 {
-	unsigned int filetype, attr = ADFS_I(inode)->attr;
+	unsigned int attr = ADFS_I(inode)->attr;
 	umode_t mode, rmask;
 	struct adfs_sb_info *asb = ADFS_SB(sb);

@@ -107,9 +94,7 @@ adfs_atts2mode(struct super_block *sb, s
 		return S_IFDIR | S_IXUGO | mode;
 	}

-	filetype = adfs_filetype(inode);
-
-	switch (filetype) {
+	switch (ADFS_I(inode)->filetype) {
 	case 0xfc0:	/* LinkFS */
 		return S_IFLNK|S_IRWXUGO;

@@ -280,7 +265,8 @@ adfs_iget(struct super_block *sb, struct
 	ADFS_I(inode)->loadaddr  = obj->loadaddr;
 	ADFS_I(inode)->execaddr  = obj->execaddr;
 	ADFS_I(inode)->attr      = obj->attr;
-	ADFS_I(inode)->stamped	  = ((obj->loadaddr & 0xfff00000) == 0xfff00000);
+	ADFS_I(inode)->filetype  = obj->filetype;
+	ADFS_I(inode)->stamped   = ((obj->loadaddr & 0xfff00000) == 0xfff00000);

 	inode->i_mode	 = adfs_atts2mode(sb, inode);
 	adfs_adfs2unix_time(&inode->i_mtime, inode);
diff -X linux-2.6.37-vanilla/Documentation/dontdiff -uprN linux-2.6.37-vanilla/fs/adfs/super.c linux-2.6.37-sks-adfs/fs/adfs/super.c
--- linux-2.6.37-vanilla/fs/adfs/super.c	2011-01-05 00:50:19.000000000 +0000
+++ linux-2.6.37-sks-adfs/fs/adfs/super.c	2011-01-21 14:18:01.000000000 +0000
@@ -143,17 +143,20 @@ static int adfs_show_options(struct seq_
 		seq_printf(seq, ",ownmask=%o", asb->s_owner_mask);
 	if (asb->s_other_mask != ADFS_DEFAULT_OTHER_MASK)
 		seq_printf(seq, ",othmask=%o", asb->s_other_mask);
+	if (asb->s_ftsuffix != 0)
+		seq_printf(seq, ",ftsuffix=%u", asb->s_ftsuffix);

 	return 0;
 }

-enum {Opt_uid, Opt_gid, Opt_ownmask, Opt_othmask, Opt_err};
+enum {Opt_uid, Opt_gid, Opt_ownmask, Opt_othmask, Opt_ftsuffix, Opt_err};

 static const match_table_t tokens = {
 	{Opt_uid, "uid=%u"},
 	{Opt_gid, "gid=%u"},
 	{Opt_ownmask, "ownmask=%o"},
 	{Opt_othmask, "othmask=%o"},
+	{Opt_ftsuffix, "ftsuffix=%u"},
 	{Opt_err, NULL}
 };

@@ -194,6 +197,11 @@ static int parse_options(struct super_bl
 				return -EINVAL;
 			asb->s_other_mask = option;
 			break;
+		case Opt_ftsuffix:
+			if (match_int(args, &option))
+				return -EINVAL;
+			asb->s_ftsuffix = option;
+			break;
 		default:
 			printk("ADFS-fs: unrecognised mount option \"%s\" "
 					"or missing value\n", p);
@@ -368,6 +376,7 @@ static int adfs_fill_super(struct super_
 	asb->s_gid = 0;
 	asb->s_owner_mask = ADFS_DEFAULT_OWNER_MASK;
 	asb->s_other_mask = ADFS_DEFAULT_OTHER_MASK;
+	asb->s_ftsuffix = 0;

 	if (parse_options(sb, data))
 		goto error;
@@ -447,11 +456,13 @@ static int adfs_fill_super(struct super_

 	root_obj.parent_id = root_obj.file_id = le32_to_cpu(dr->root);
 	root_obj.name_len  = 0;
-	root_obj.loadaddr  = 0;
-	root_obj.execaddr  = 0;
+	/* Set root object date as 01 Jan 1987 00:00:00 */
+	root_obj.loadaddr  = 0xfff0003f;
+	root_obj.execaddr  = 0xec22c000;
 	root_obj.size	   = ADFS_NEWDIR_SIZE;
 	root_obj.attr	   = ADFS_NDA_DIRECTORY   | ADFS_NDA_OWNER_READ |
 			     ADFS_NDA_OWNER_WRITE | ADFS_NDA_PUBLIC_READ;
+	root_obj.filetype  = -1;

 	/*
 	 * If this is a F+ disk with variable length directories,
@@ -466,6 +477,13 @@ static int adfs_fill_super(struct super_
 		asb->s_namelen = ADFS_F_NAME_LEN;
 	}

+	/*
+	 * ,xyz hex filetype suffix may be added by driver
+	 * to files that have valid RISC OS filetype
+	 */
+	if (asb->s_ftsuffix)
+		asb->s_namelen += 4;
+
 	root = adfs_iget(sb, &root_obj);
 	sb->s_root = d_alloc_root(root);
 	if (!sb->s_root) {

Re: [PATCH] adfs: add hexadecimal filetype suffix option

[Arnd Bergmann]

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.
> 
> 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 <stuart.swales.croftnuisk@gmail.com>

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.

	Arnd

Re: [PATCH] adfs: add hexadecimal filetype suffix option

[Russell King]

On Fri, Jan 21, 2011 at 05:47:17PM +0100, Arnd Bergmann wrote:
> 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.
> > 
> > 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 <stuart.swales.croftnuisk@gmail.com>
> 
> 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.

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.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: [PATCH] adfs: add hexadecimal filetype suffix option

[Arnd Bergmann]

On Friday 21 January 2011 18:26:58 Russell King wrote:
> On Fri, Jan 21, 2011 at 05:47:17PM +0100, Arnd Bergmann wrote:
>
> > 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.

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 <arnd@arndb.de>
Cc: Russell King <rmk@arm.linux.org.uk>
Cc: Stuart Swales <stuart.swales.croftnuisk@gmail.com>
---

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

diff --git a/fs/adfs/dir.c b/fs/adfs/dir.c
index 3b4a764..3d83075 100644
--- a/fs/adfs/dir.c
+++ b/fs/adfs/dir.c
@@ -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);
 }
diff --git a/fs/adfs/inode.c b/fs/adfs/inode.c
index 65794b8..09fe401 100644
--- a/fs/adfs/inode.c
+++ b/fs/adfs/inode.c
@@ -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;
 }
diff --git a/fs/adfs/super.c b/fs/adfs/super.c
index 2d79540..06d7388 100644
--- a/fs/adfs/super.c
+++ b/fs/adfs/super.c
@@ -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;
 }
 

Re: [PATCH] adfs: add hexadecimal filetype suffix option

[Stuart Swales]

Sure, will give it a thrash on SMP system this weekend.

Stuart

On 21/01/2011 22:02, Arnd Bergmann wrote:
> On Friday 21 January 2011 18:26:58 Russell King wrote:
>> On Fri, Jan 21, 2011 at 05:47:17PM +0100, Arnd Bergmann wrote:
>>
>>> 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.
>
> 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<arnd@arndb.de>
> Cc: Russell King<rmk@arm.linux.org.uk>
> Cc: Stuart Swales<stuart.swales.croftnuisk@gmail.com>
> ---
>
>   fs/adfs/dir.c   |    6 ------
>   fs/adfs/inode.c |    6 ------
>   fs/adfs/super.c |   13 +------------
>   3 files changed, 1 insertions(+), 24 deletions(-)
>
> diff --git a/fs/adfs/dir.c b/fs/adfs/dir.c
> index 3b4a764..3d83075 100644
> --- a/fs/adfs/dir.c
> +++ b/fs/adfs/dir.c
> @@ -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);
>   }
> diff --git a/fs/adfs/inode.c b/fs/adfs/inode.c
> index 65794b8..09fe401 100644
> --- a/fs/adfs/inode.c
> +++ b/fs/adfs/inode.c
> @@ -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;
>   }
> diff --git a/fs/adfs/super.c b/fs/adfs/super.c
> index 2d79540..06d7388 100644
> --- a/fs/adfs/super.c
> +++ b/fs/adfs/super.c
> @@ -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;
>   }
>


-- 
Stuart Swales

Re: [PATCH] adfs: add hexadecimal filetype suffix option

[Stuart Swales]

On Wed, 19 Jan 2011, Andrew Morton wrote:

> On Wed, 12 Jan 2011 18:07:23 +0000
> Stuart Swales <stuart.swales.croftnuisk@gmail.com> wrote:
>
> > From: Stuart Swales<stuart.swales.croftnuisk@gmail.com>
> >
> > 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.
> >
>
> 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.

Yes, indeed it would!  This is needed so we can store RISC OS file type
info on non-native systems.  This has been useful in the past to
share files over NFS and will be useful in future when migrating RISC
OS apps and data over onto virtualized systems such as the RISC OS
emulator RPCemu.


> 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?

> 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.

Done.  Thanks for the pointer.


> 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.


I blame icedove again!

Cheers,

Stuart Swales

Re: [PATCH] adfs: add hexadecimal filetype suffix option

[Andrew Morton]

On Fri, 21 Jan 2011 14:43:14 +0000 (GMT) Stuart Swales <stuart.swales.croftnuisk@gmail.com> wrote:

> > 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?

It should work and should be tested and maintained.

I do worry about the effect on userspace of having all the filenames
change on the fly.  I guess if it makes applications misbehave then
that's a userspace (administrator) error.

Does it mean that existing dcache entries are now out of sync, so some
filenames are suffixless and others are suffixed?  If so, that might be
too hard to fix, and some way of disabling `mount -o remount' should be
found.

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

That too.

Re: [PATCH] adfs: add hexadecimal filetype suffix option

[Geert Uytterhoeven]

On Thu, Jan 20, 2011 at 00:49, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Wed, 12 Jan 2011 18:07:23 +0000
> Stuart Swales <stuart.swales.croftnuisk@gmail.com> wrote:
>> From: Stuart Swales<stuart.swales.croftnuisk@gmail.com>

>> +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

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] adfs: add hexadecimal filetype suffix option

[Al Viro]

On Wed, Mar 23, 2011 at 09:36:48PM +0100, Geert Uytterhoeven wrote:
> On Thu, Jan 20, 2011 at 00:49, Andrew Morton <akpm@linux-foundation.org> wrote:
> > On Wed, 12 Jan 2011 18:07:23 +0000
> > Stuart Swales <stuart.swales.croftnuisk@gmail.com> wrote:
> >> From: Stuart Swales<stuart.swales.croftnuisk@gmail.com>
> 
> >> +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

And it's absolutely right - unsigned short it promoted to int as part of
usual arithmetic conversions and the value is not changed.  And it's *not*
going to be equal to -1.

Re: [PATCH] adfs: add hexadecimal filetype suffix option

[Andrew Morton]

On Wed, 23 Mar 2011 21:36:48 +0100
Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> On Thu, Jan 20, 2011 at 00:49, Andrew Morton <akpm@linux-foundation.org> wrote:
> > On Wed, 12 Jan 2011 18:07:23 +0000
> > Stuart Swales <stuart.swales.croftnuisk@gmail.com> wrote:
> >> From: Stuart Swales<stuart.swales.croftnuisk@gmail.com>
> 
> >> +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
> 

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.

Re: [PATCH] adfs: add hexadecimal filetype suffix option

[Stuart Swales]

The file typing code is using -1 (truncated to __u16) to represent the 
case when no explicit 12-bit file type was set on the file, such as is 
the case where the file has explicit 32-bit load and execute addresses 
(Acorn specific legacy stuff, started with the BBC Micro).  It would 
seem to be best if the -1 were replaced by 0xffffu.

Stuart

On 23/03/2011 20:58, Andrew Morton wrote:
> On Wed, 23 Mar 2011 21:36:48 +0100
> Geert Uytterhoeven<geert@linux-m68k.org>  wrote:
>
>> On Thu, Jan 20, 2011 at 00:49, Andrew Morton<akpm@linux-foundation.org>  wrote:
>>> On Wed, 12 Jan 2011 18:07:23 +0000
>>> Stuart Swales<stuart.swales.croftnuisk@gmail.com>  wrote:
>>>> From: Stuart Swales<stuart.swales.croftnuisk@gmail.com>
>>
>>>> +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
>>
>
> 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.
>


-- 
Stuart Swales