[PATCH] ASFS filesystem driver, kernel 2.4.xx and 2.6.x

[PATCH] ASFS filesystem driver, kernel 2.4.xx and 2.6.x

[Marek Szyprowski]

This patch adds full support for Amiga SmartFileSystem. This filesystem
is being used very commonly on AmigaOS and MorphOS systems. This driver
could be compiled in safe (both for user and filesystem data) read-only
mode as well as more dangerous read-write mode. Read-write mode has been
tested on Linux/PPC for over a half of the year and I believe all critical
bugs has been removed. The filesystem itself haven't been corrupted any
time. I also did some test on Linux/x86 machine and it also worked without
problems, so driver should be endianes-clean. I don't know if it works on
64bit systems like Linux/Alpha or so, because I don't have access to such
systems. 

Patch for 2.4.xx kernel tree is available on
http://home.elka.pw.edu.pl/~mszyprow/programy/asfs/asfs-1.0b4_patch_2.4.21.diff.gz

Patch for 2.6.x kernel tree is available on
http://home.elka.pw.edu.pl/~mszyprow/programy/asfs/asfs-1.0b6_patch_2.6.3.diff.gz

ASFS driver project homepage:
http://freshmeat.net/projects/asfs_linuxdriver/

Any feedback and comments are welcome. 

Regards
-- 
Marek Szyprowski .. GG:2309080 .. mailto:marek@amiga.pl ..
...... happy AmigaOS, MacOS and Debian/Linux user ........
........... http://march.home.staszic.waw.pl/ ............

Re: [PATCH] ASFS filesystem driver, kernel 2.4.xx and 2.6.x

[Christoph Hellwig]

 - asfs_fs.h, asfs_fs_i.h and asfs_fs_sb.h don't belong to include/linux
   but fs/asfs (and get saner names while you're at it)
 - there's a bunch of files completely under ifdef CONFIG_ASFS_RW, just
   make them depend on that in the makefile instead using

	asfs-y			+= dir.o ...
	asfs-$(CONFIG_ASFS_RW)	+= adminspace.o bitfuncs.o ...

 - names like setfreeblocks are a little too generic, please give them
   asfs_ prefixes
 - you probably want to replace the routines in bitfuncs.c with usage of
   linux-native routines from bitops.h like find_first_bit(), etc wherever
   possible
 - asfs_parse_options should use lib/parser.c helpers
 - in asfs_fill_super you're leaking if d_alloc_root fails
 - you're using lock_super for a lot of internal synchronization, but you
   should implement your own locking instead and document what each lock
   protects.
 - the filp->f_count checks in ->open and ->close are totally bogus
 - asfs_file_release looks very fishy.  Why do you set ->datemodified
   on close, not on actual modification?
 - the whole handling of ->modified and reading the object from disk
   again when you want to write it back looks bogus.  Why don't you
 - ->create, ->mkdir and ->mknod seem to share most of them code, shouldn't
   you use a common subroutine?  Mayube dito for ->unlink and ->rmdir
 - please call asfs_delete asfs_unlink to match the method name.
 - this whole rececycled bin stuff makes me sick.  Where is that handling
   encoded in the ondisk format?  Can't you just do normal unix-style unlinks?

Re: ASFS filesystem driver, kernel 2.4.xx and 2.6.x

[Marek Szyprowski]

Hello Christoph

Thanks for your comments! This is my first file system driver, so forgive me
some mistakes :)

On  7.06.04, you wrote:

>  - asfs_fs.h, asfs_fs_i.h and asfs_fs_sb.h don't belong to
>    include/linux but fs/asfs (and get saner names while you're at it)

well, they are there as a direct adaptation of 2.4.xx version of driver, but
I will move them to fs/asfs in 2.6.x version.

>  - there's a bunch of files completely under ifdef CONFIG_ASFS_RW, just
>    make them depend on that in the makefile instead using
>     asfs-y            += dir.o ...
>     asfs-$(CONFIG_ASFS_RW)    += adminspace.o bitfuncs.o ...

OK. 

>  - names like setfreeblocks are a little too generic, please give them
>    asfs_ prefixes

OK. I assume that this concerns only non static function names.

>  - you probably want to replace the routines in bitfuncs.c with usage
>    of linux-native routines from bitops.h like find_first_bit(), etc
>    wherever possible
>  - asfs_parse_options should use lib/parser.c helpers

I will try.

>  - you're using lock_super for a lot of internal synchronization, but
>    you should implement your own locking instead and document what each
>    lock protects.

Are there any disadvantedes for whole kernel/system of useing lock_super? I
used it because almost for every operation almost whole filesystem need to
be locked... Introducing custom locks would need really much work and there
won't be any measurable speed increase IMHO.

>  - the filp->f_count checks in ->open and ->close are totally bogus

Ok. So how should I check for file closing? My driver do some free blocks
preallocation during file write. When writing is finished (file is closed)
I need to free all prealocated blocks and fix some object data (like file
size for example). Where should I call a function doing this? I decided to
do preallocation of file blocks, because allocation of one single block on
asfs disk is really expensive (driver needs to find last used "extend" in
extend tree, check in bitmap if it could be expanded, allocate new block in
bitmat and fix or add "extend" to extend tree). 

>  - asfs_file_release looks very fishy.  Why do you set ->datemodified
>    on close, not on actual modification?
>  - the whole handling of ->modified and reading the object from disk
>    again when you want to write it back looks bogus.

Well it is like that the first look. I really need to reread object on every
itd modification, because it COULD change its place on disk (in each
"ObjectContainer" there are several "Objects" and they move if new object
are added or removed to/from container). Because rereading object take some
time I decided to modify its ->datemodified only on close. This really
doesn't do much difference and shouldn't be a problem IMHO.

>  - ->create, ->mkdir and ->mknod seem to share most of them code,
>    shouldn't you use a common subroutine?  Mayube dito for ->unlink and
>  ->rmdir 

OK.

>- please call asfs_delete asfs_unlink to match the method  name. 

OK.

>- this whole rececycled bin stuff makes me sick.  Where is that
>    handling encoded in the ondisk format?  Can't you just do normal
>    unix-style unlinks?

What do you mean by "unix-style unlinks"? I don't understand your question
about recycled bin...

Regards
-- 
Marek Szyprowski .. GG:2309080 .. mailto:marek@amiga.pl ..
...... happy AmigaOS, MacOS and Debian/Linux user ........
........... http://march.home.staszic.waw.pl/ ............

Re: ASFS filesystem driver, kernel 2.4.xx and 2.6.x

[Marek Szyprowski]

Hello Christoph

According to Your advice I cleaned-up the code. It is available on:

http://home.elka.pw.edu.pl/~mszyprow/programy/asfs/

The new patch:

http://home.elka.pw.edu.pl/~mszyprow/programy/asfs/asfs-1.0b7_patch_2.6.3.diff.gz

and the diff against previous version:

http://home.elka.pw.edu.pl/~mszyprow/programy/asfs/asfs-1.0b6_to_1.0b7_patch_2.6.3.diff.gz

Regards
-- 
Marek Szyprowski .. GG:2309080 .. mailto:marek@amiga.pl ..
...... happy AmigaOS, MacOS and Debian/Linux user ........
........... http://march.home.staszic.waw.pl/ ............