RE: [lustre mess] is mgc_fs_setup() reachable at all?

[Peng, Tao]

On Fri, Jul 19, 2013 at 4:12 PM, Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
> On Thu, Jul 18, 2013 at 02:57:11PM -0600, Andreas Dilger wrote:
>
>> > _THAT_ was going to be a remotely supplied data?  I really hope I've
>> > misparsed what you said above...
>> >
>> > And that still leaves the question about the code path that could
>> > lead to execution of mgc_fs_setup().
>>
>> The KEY_SET_FS is only used in the server code, not on the client.
>> The MGC code is shared between client and server to mount the
>> filesystem and fetch the cluster configuration from the management
>> server.  In the case of a server mount, it also has to mount the
>> underlying block device, which isn't true on the client, so this
>> code is indeed unused.
>
> Wait a minute...  So we have client side of things in staging, with
> parts shared with the server, which is *not* in tree at all?  That
> sounds painful - any changes done to the client code either risk
> to break the server, or have the copies of the shared stuff diverge...
>
Currently we are hoping that the divergent part can be kept small and maintainable. Patches are ported between kernel and external trees to keep them in sync. There are also ongoing work to split client and server code. In external tree, server code are mostly marked by HAVE_SERVER_SUPPORT macro if they are shared with client but not used by client. Currently we removed such code that are not used by client in kernel Lustre but those are mostly whole functions. For the cases like in mgc_fs_setup(), can we use #ifdef HAVE_LUSTRE_SERVER_SUPPORT to mark out the unused code, or keep it as is, or do you want us to remove it completely?

> I honestly have no idea about your plans wrt merging; are you going
> to put the server side of things there as well?
>
Client is first attempted because it is easier to put into kernel, compared with server that still needs patching other pieces of kernel for a few places. I'm not sure about plans for server though. Andreas may know more about server plans.

> Another thing: is your ll_statfs_internal() safe to call right up to
> the moment when client_common_put_super calls lprocfs_unregister_mountpoint?
> Because procfs IO *can* come right until the procfs entry removal;
> said removal will act as a barrier, so it won't leak past the return from
> remove_proc_entry(), but that's it.
>
You are right. It seems problematic. We should really remove procfs entry before cleaning up obd export in client_common_put_super(). Something like below:

diff --git a/drivers/staging/lustre/lustre/llite/llite_lib.c b/drivers/staging/lustre/lustre/llite/llite_lib.c
index afae801..8e89311 100644
--- a/drivers/staging/lustre/lustre/llite/llite_lib.c
+++ b/drivers/staging/lustre/lustre/llite/llite_lib.c
@@ -685,6 +685,9 @@ void client_common_put_super(struct super_block *sb)
 	}
 #endif
 
+	/* remove proc entry first to prevent any new callers */
+	lprocfs_unregister_mountpoint(sbi);
+
 	ll_close_thread_shutdown(sbi->ll_lcq);
 
 	cl_sb_fini(sb);
@@ -698,8 +701,6 @@ void client_common_put_super(struct super_block *sb)
 	 * see LU-2543. */
 	obd_zombie_barrier();
 
-	lprocfs_unregister_mountpoint(sbi);
-
 	obd_fid_fini(sbi->ll_md_exp->exp_obd);
 	obd_disconnect(sbi->ll_md_exp);
 	sbi->ll_md_exp = NULL;

> While we are at procfs side of thing, looks like you need exclusion between
> ll_..._seq_write() that clear/set bits in ->ll_flags; at least I haven't
> found anything that would prevent the races among those.
Yes, that part needs to be fixed as well, by taking sbi->ll_lock.

Thanks very much for looking at it. Will send patches to fix the issues.

Thanks,
Tao
èº{.nÇ+‰·Ÿ®‰­†+%ŠËlzwm…ébëæìr¸›zX§»®w¥Š{ayºÊÚë,j­¢f£¢·hš‹àz¹®w¥¢¸¢·¦j:+v‰¨ŠwèjØm¶Ÿÿ¾«‘êçzZ+ƒùšŽŠÝj"ú!¶iO•æ¬z·švØ^¶m§ÿðÃnÆàþY&—