Thread (3 messages) read the whole thread 3 messages, 3 authors, 2007-12-17

Re: 1st version of azfs

From: Jan Engelhardt <hidden>
Date: 2007-12-17 19:29:05
Also in: linux-mm, lkml

+config AZ_FS
+	tristate "AZFS filesystem support"
+	default m
I do not think it should default to anything.
+#define AZFS_SUPERBLOCK_FLAGS		MS_NOEXEC | \
+					MS_SYNCHRONOUS | \
+					MS_DIRSYNC | \
+					MS_ACTIVE

+#define AZFS_BDI_CAPABILITIES		BDI_CAP_NO_ACCT_DIRTY | \
+					BDI_CAP_NO_WRITEBACK | \
+					BDI_CAP_MAP_COPY | \
+					BDI_CAP_MAP_DIRECT | \
+					BDI_CAP_VMFLAGS
+
+#define AZFS_CACHE_FLAGS		SLAB_HWCACHE_ALIGN | \
+					SLAB_RECLAIM_ACCOUNT | \
+					SLAB_MEM_SPREAD
+
Suggest () around the (MS_NOEXEC|...|...)
+enum azfs_direction {
+	AZFS_MMAP,
+	AZFS_READ,
+	AZFS_WRITE
+};
+
+struct azfs_super {
+	struct list_head		list;
+	unsigned long			media_size;
+	unsigned long			block_size;
+	unsigned short			block_shift;
+	unsigned long			sector_size;
+	unsigned short			sector_shift;
+	unsigned long			ph_addr;
+	unsigned long			io_addr;
+	struct block_device		*blkdev;
+	struct dentry			*root;
+	struct list_head		block_list;
+	rwlock_t			lock;
+};
Some of these probably should be sometypedef_t or so, to ensure they
have their minimum width on 32-bit. The struct also could have some
reordering to avoid needless padding.
+struct azfs_block {
+	struct list_head		list;
+	unsigned long			id;
+	unsigned long			count;
+};
+
Same. unsigned long <=> uint64_t might be needed/helpful/etc.
+static struct azfs_super_list		super_list;
+static struct kmem_cache		*azfs_znode_cache __read_mostly = NULL;
+static struct kmem_cache		*azfs_block_cache __read_mostly = NULL;
NULL is implicit, drop it, save some bytes in the object file.
+static int
+azfs_mknod(struct inode *dir, struct dentry *dentry, int mode, dev_t dev)
+{
+	struct inode *inode;
+
+	inode = azfs_new_inode(dir->i_sb, dir, mode, dev);
+	if (!inode)
+		return -ENOSPC;
+
+	if (S_ISREG(mode))
+		I2Z(inode)->size = 0;
+
+	dget(dentry);
+	d_instantiate(dentry, inode);
+
+	return 0;
+}
Either azfs_mknod(), azfs_new_inode() or init_special_inode() seems
to be missing settings ->size to 0 in the !S_IFREG case and
setting ->size to something good-looking for S_IFDIR.
+/**
+ * azfs_open - open() method for file_operations
+ * @inode, @file: see file_operations methods
+ */
+static int
+azfs_open(struct inode *inode, struct file *file)
+{
+	file->private_data = inode;
+
+	if (file->f_flags & O_TRUNC) {
+		i_size_write(inode, 0);
+		inode->i_op->truncate(inode);
+	}
+	if (file->f_flags & O_APPEND)
+		inode->i_fop->llseek(file, 0, SEEK_END);
+
+	return 0;
+}
This looks like duplicate code. Usually the generic fs functions take
care of that, including quota handling which seems to be missing
here if this is continuing to exist.
+	page_prot = pgprot_val(vma->vm_page_prot);
+	page_prot |= (_PAGE_NO_CACHE | _PAGE_RW);
redundant ().
+			for_each_block(ding, &super->block_list) {
+				if (!west && (ding->id + ding->count == id))
+					west = ding;
+				else if (!east && (id + count == ding->id))
+					east = ding;
redundant().
+static struct inode*
+azfs_new_inode(struct super_block *sb, struct inode *dir, int mode, dev_t dev)
+{
+	struct inode *inode;
+
+	inode = new_inode(sb);
+	if (!inode)
+		return NULL;
+
+	inode->i_atime = inode->i_mtime = inode->i_ctime = CURRENT_TIME;
+
+	inode->i_mode = mode;
+	if (dir) {
+		dir->i_mtime = dir->i_ctime = inode->i_mtime;
+		inode->i_uid = current->fsuid;
+		if (dir->i_mode & S_ISGID) {
+			if (S_ISDIR(mode))
+				inode->i_mode |= S_ISGID;
+			inode->i_gid = dir->i_gid;
+		} else {
+			inode->i_gid = current->fsgid;
+		}
+	} else {
+		inode->i_uid = 0;
+		inode->i_gid = 0;
+	}
Why not fsuid/fsgid in the else case?
+azfs_statfs(struct dentry *dentry, struct kstatfs *stat)
+{
[...]
+	mutex_lock(&sb->s_lock);
+	list_for_each_entry(inode, &sb->s_inodes, i_sb_list) {
+		inodes++;
+		blocks += inode->i_blocks;
+	}
+	mutex_unlock(&sb->s_lock);
Can this be improved somehow? If the list of inodes is long, doing
statvfs() may keep the filesystem real busy.
+static struct super_operations azfs_ops = {
+	.alloc_inode	= azfs_alloc_inode,
+	.destroy_inode	= azfs_destroy_inode,
+	.drop_inode	= generic_delete_inode,
+	.delete_inode	= azfs_delete_inode,
+	.statfs		= azfs_statfs
+};
Trailing comma preferred.
+azfs_fill_super(struct super_block *sb, void *data, int silent)
+{
+	if (!disk || !disk->queue) {
+		printk(KERN_ERR "%s needs a block device which has a gendisk "
+				"with a queue\n",
+				AZFS_FILESYSTEM_NAME);
+		return -ENOSYS;
+	}
ENOSYS seems inappropriate.
+	if (!get_device(disk->driverfs_dev)) {
+		printk(KERN_ERR "%s cannot get reference to device driver\n",
+				AZFS_FILESYSTEM_NAME);
+		return -EFAULT;
+	}
as does EFAULT.
+static struct file_system_type azfs_fs = {
+	.owner		= THIS_MODULE,
+	.name		= AZFS_FILESYSTEM_NAME,
I see you have made plans to change the filesystem name :)
+	.get_sb		= azfs_get_sb,
+	.kill_sb	= azfs_kill_sb,
+	.fs_flags	= AZFS_FILESYSTEM_FLAGS
or just replace these macros.
+static int __init
+azfs_init(void)
+{
C'mon, that fits on one line.
+	if (!azfs_znode_cache) {
+		printk(KERN_ERR "Could not allocate inode cache for %s\n",
+				AZFS_FILESYSTEM_NAME);
While we are at it,
		printk(KERN_ERR "Could not blafasel for " AZFS_FILESYSTEM_NAME "\n");
saves the extra argument.
+{
+	struct azfs_super *super, *PILZE;
Process forests, superblock mushrooms, GNOME desktop, what next?
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help