[SSI-devel] Re: Analysis of ssi_shmget() panic in 2.6 SSI kernel

[John B. <joh...@hp...>]

SAMPATHKUMAR KISHORE KANIYAR wrote:
>  Hello John,
>  
>  As related to the cfs_shmget() panic on 2.6 SSI kernel, after looking
>  through the panic stack trace and the register values, the initial
>  reason for panic appeared (to be obvious) due to the following code
>  in cfs/inode.c:
>  
>  cfs_hpget()
>      |
>      v
>  iget5_locked+0xca/0x150
>      |
>      v
>  get_new_inode+0x15/0x1a0
>      |
>      v
>  alloc_inode+0x146
>  
>  cfs_shm_sb, which was eventually being passed to alloc_inode(), was NULL,
>  and hence the panic.
>  
>  cfs_shm_sb, which is setup in cfs_shm_init() in cfs/cfs_ipcshm.c is
>  under #ifdef SSI_XXX and we have not turned it on.
>  
>  I and Bharata worked on fixing this, and as part it, we turned the above
>  #ifdef on. It still panic'ed, now at include/cluster/ssi/super.h, due
>  to the invocation of sgetx() in cfs_stackmount(), after:
>  
>  #warning cfs_stackmount() can be called for non-block device
>  filesystems, too.
>  
>  So, we reasoned that it was incorrect to pass test_bdev_super and
>  set_bdev_super as parameters when this (shm instance) is not a bdev.
>  Hence we added the below code:
>  
>  ----- BEGIN: PATCH CHANGES --------
>  
>  #warning cfs_stackmount() can be called for non-block device
>  filesystems, too.
>  +if (bdev) {
>   	down(&bdev->bd_mount_sem);
>   	csb = sgetx(cfs_fstype, test_bdev_super, set_bdev_super, bdev, TRUE,
>   		psb->s_flags);
>   	up(&bdev->bd_mount_sem);
>  +} else {
>  +	csb = sgetx(cfs_fstype, NULL, set_anon_super, NULL, TRUE,
>  +		psb->s_flags);
>  +}
>   	if (IS_ERR(csb))
>   		return 0;
>  
>  @@ -140,9 +146,11 @@
>   
>   	(void)ssidev_get_s_ssidev(csb, 1);
>   	csb->s_flags = psb->s_flags & (~MS_CFS);
>  +if (bdev) {
>   	strlcpy(csb->s_id, bdevname(bdev, b), sizeof(csb->s_id));
>   	csb->s_old_blocksize = block_size(bdev);
>   	sb_set_blocksize(csb, csb->s_old_blocksize);
>  +}
>   
>   	data.magic = CFS_MOUNT_MAGIC;
>   	data.mode = CFS_SVR_ON_CLI;
>  
>  
>  ----- END: PATCH CHANGES --------
>  
>  After the above, we no longer see the shmget related crash; However, we
>  are still seeing a different panic in page fault handler which we believe
>  is due to more ssi_shmget() related codepath exercised due to the above
>  changes.
>  
>  We are working on it, and hope to get to the bottom of it tomorrow.
>  
>  Are we in the right direction?

Well, the indentation is ugly. The idea looks okay as far as I can tell 
without testing it myself.

If we want to stack on "single" filesystems (devfs was the only one in 
2.4, I think, and I don't know if we are going to need to in 2.6, yet.) 
more than your fix will be needed --- we'd probably want to pass in the 
test routine to be used --- but we can ignore this for now.

>  
>  
>  Also, wanted to know, there exists a comment in kernel/fs/super.c,
>  in get_sb_single():
>  
>  #ifdef CONFIG_CFS
>          /* SSI_XXX: This may be a problem here.  In 2.4 code we used
>           * get_anon_super() which could have different semantics than
>           * get_sb_bdev().  Now this is common code in sgetx().
>           */
>          s = sgetx(fs_type, compare_single, set_anon_super, NULL, !iscfs(flags),
>                  flags);
>  #else
>          s = sget(fs_type, compare_single, set_anon_super, NULL);
>  #endif
>  
>  What problem is being referred to above? Can you briefly explain?

I looked at the code and couldn't see a problem. I talked to Dave and he 
believed he left this comment around to indicate that someone needed to 
look and make sure there wasn't a problem, not that there was one. I 
think we can delete the comment.

>  
>  NOTE: I and Bharata are somewhat familiar with the related code since 
>        we have been going through that for some days now.
>  
>  Regards,
>  - Kishore
> 

John