Thank you very much Christoph for your comments. Some of them I have already found useful and fixed my code accordinly. Some other your comments I find interesting but would like to investigate them a little bit more.
I will send an updated version of my patch soon.
> truncate and seeking are done by the VFS, not need to do it.
I think VFS is doing it not exactly the same way which I need. But I will look at it once more again to make sure.
> Also no need to stuff the inode in file->private_data because
> it's always available through file->f_path.dentry->inode.
Fixed.
> You need to check for an NULL pointer from kmem_cache_alloc
> here.
Fixed.
> > + if (!disk || !disk->queue) {
>
> This won't ever be zero, no need to check.
Agree 50%. I have removed a printk from here but have left the following line;
BUG_ON(!disk || !disk->queue);
> > + if (!get_device(disk->driverfs_dev)) {
> > + printk(KERN_ERR "%s cannot get reference to device driver\n",
> > + AZFS_FILESYSTEM_NAME);
> > + return -EFAULT;
> > + }
>
> You don't need another reference, the disk won't go away while the
> block device is open.
Not agree. I leave get_device here but remove the printk.
> > + spin_lock(&super_list.lock);
> > + list_for_each_entry(knoten, &super_list.head, list)
> > + if (knoten->blkdev == sb->s_bdev) {
> > + super = knoten;
> > + break;
> > + }
> > + spin_unlock(&super_list.lock);
>
> This can't happen. get_sb_bdev already searches for the same superblock
> already existing and doesn't even call into fill_super in that case.
I will check it.
> > +static void
> > +azfs_kill_sb(struct super_block *sb)
> > +{
> > + sb->s_root = NULL;
>
> Very bad idea, if you set sb->s_root to zero before calling
> generic_shutdown_super it will miss a lot of the taerdown activity.
I need it because I want to keep all super blocks and inodes to make it possible to mount the same AZFS partition later and to let user see all his files again. Don't forget - AZFS keeps all the inode data in RAM.
> > + spin_lock(&super_list.lock);
> > + list_for_each_entry_safe(super, SUPER, &super_list.head, list) {
> > + disk = super->blkdev->bd_disk;
> > + list_del(&super->list);
> > + iounmap((void*) super->io_addr);
> > + write_lock(&super->lock);
> > + for_each_block_safe(block, knoten, &super->block_list)
> > + azfs_block_free(block);
> > + write_unlock(&super->lock);
> > + disk->driverfs_dev->driver_data = NULL;
> > + disk->driverfs_dev->platform_data = NULL;
> > + kfree(super);
> > + put_device(disk->driverfs_dev);
>
> All this teardown should happen in ->put_super, and with this and
> the above comment there should be need for a list of all superblocks.
Same thing - super blocks and inodes of unmounted file systems are still in RAM.
--
Mit freundlichen Grüßen / met vriendelijke groeten / avec regards
Maxim V. Shchetynin
Linux Kernel Entwicklung
IBM Deutschland Entwicklung GmbH
Linux für Cell, Abteilung 3250
Schönaicher Straße 220
71032 Böblingen
Vorsitzender des Aufsichtsrats: Johann Weihen
Geschäftsführung: Herbert Kircher
Sitz der Gesellschaft: Böblingen
Registriergericht: Amtsgericht Stuttgart, HRB 243294
Fahr nur so schnell wie dein Schutzengel fliegen kann!
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
