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

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

[Al Viro]

[nathan cc'd since he's the only person mentioned in mgc_request.c]

One general observation: drivers/taging/lustre is highly reviewer-hostile...

Found by unrelated grep:

drivers/staging/lustre/lustre/mgc/mgc_request.c:1040:		if (vallen != sizeof(struct super_block))

Huh?  The code around that line looks so:
	if (KEY_IS(KEY_SET_FS)) {
		struct super_block *sb = (struct super_block *)val;
		struct lustre_sb_info *lsi;
		if (vallen != sizeof(struct super_block))
			RETURN(-EINVAL);
		lsi = s2lsi(sb);
		rc = mgc_fs_setup(exp->exp_obd, sb, lsi->lsi_srv_mnt);
		if (rc) {
			CERROR("set_fs got %d\n", rc);
		}
		RETURN(rc);
	}
What is going on here?  We cast something to struct super_block *?
Where does it come from?  The function it's in is

int mgc_set_info_async(const struct lu_env *env, struct obd_export *exp,
		       obd_count keylen, void *key, obd_count vallen,
		       void *val, struct ptlrpc_request_set *set)

which says damn little, other than "multiplexor with no type safety".  Who
calls that?
; git grep -n -w mgc_set_info_async
drivers/staging/lustre/lustre/mgc/mgc_request.c:1001:int mgc_set_info_async(const struct lu_env *env, struct obd_export *exp,
drivers/staging/lustre/lustre/mgc/mgc_request.c:1836:	.o_set_info_async = mgc_set_info_async,
;

Umm...  OK, looks like it's only called as a method.  Unfortunately, grep for
o_set_info_async brings a lot of instances and no callers.  After a lot of
cursing, the following antisocial Fine Piece Of Code had been located:

#define OBP(dev, op)    (dev)->obd_type->typ_dt_ops->o_ ## op

along with

        rc = OBP(exp->exp_obd, set_info_async)(env, exp, keylen, key, vallen,
                                               val, set);

As far as I'm concerned, macros like that are equivalent to spitting into
reviewers' eyes.  Sometimes out of malice, sometimes just because the authors
felt like it would be a cute prank, either way it makes life really unpleasant -
when you need to guess which part of method/function name is to be searched for
to locate the call sites... it gets old very soon.
Anyway, this thing is in

static inline int obd_set_info_async(const struct lu_env *env,
				     struct obd_export *exp, obd_count keylen,
				     void *key, obd_count vallen, void *val,
				     struct ptlrpc_request_set *set)

Again, the type safety is inexistent, but let's cross the fingers and look for
callers (and hope they hadn't pulled a similar cute trick with ## here)

; git grep -n -w obd_set_info_async
drivers/staging/lustre/lustre/include/obd_class.h:508:static inline int obd_set_info_async(const struct lu_env *env,
drivers/staging/lustre/lustre/ldlm/ldlm_lockd.c:522:		rc = obd_set_info_async(req->rq_svc_thread->t_env,
drivers/staging/lustre/lustre/llite/dir.c:658:	rc = obd_set_info_async(NULL, mgc, sizeof(KEY_SET_INFO), KEY_SET_INFO,
drivers/staging/lustre/lustre/llite/llite_lib.c:558:	err = obd_set_info_async(NULL, sbi->ll_dt_exp, sizeof(KEY_CHECKSUM),
drivers/staging/lustre/lustre/llite/llite_lib.c:563:	err = obd_set_info_async(NULL, sbi->ll_dt_exp, sizeof(KEY_CACHE_SET),
drivers/staging/lustre/lustre/llite/llite_lib.c:1964:	obd_set_info_async(NULL, sbi->ll_md_exp,
drivers/staging/lustre/lustre/llite/llite_lib.c:1967:	obd_set_info_async(NULL, sbi->ll_dt_exp,
drivers/staging/lustre/lustre/llite/llite_lib.c:2032:		err = obd_set_info_async(NULL, sbi->ll_md_exp,
drivers/staging/lustre/lustre/llite/lproc_llite.c:437:		rc = obd_set_info_async(NULL, sbi->ll_dt_exp,
drivers/staging/lustre/lustre/llite/lproc_llite.c:485:	rc = obd_set_info_async(NULL, sbi->ll_dt_exp, sizeof(KEY_CHECKSUM),
drivers/staging/lustre/lustre/lmv/lmv_obd.c:281:		obd_set_info_async(NULL, tgt->ltd_exp, sizeof(KEY_INTERMDS),
drivers/staging/lustre/lustre/lmv/lmv_obd.c:2239:			err = obd_set_info_async(env, tgt->ltd_exp,
drivers/staging/lustre/lustre/lov/lov_obd.c:638:		rc = obd_set_info_async(NULL, tgt->ltd_exp,
drivers/staging/lustre/lustre/lov/lov_obd.c:2629:			err = obd_set_info_async(env, tgt->ltd_exp,
drivers/staging/lustre/lustre/lov/lov_obd.c:2633:			err = obd_set_info_async(env, tgt->ltd_exp,
drivers/staging/lustre/lustre/lov/lov_obd.c:2646:			err = obd_set_info_async(env, tgt->ltd_exp, keylen,
drivers/staging/lustre/lustre/lov/lov_obd.c:2655:			err = obd_set_info_async(env, tgt->ltd_exp,
drivers/staging/lustre/lustre/mdc/mdc_request.c:1767:		rc = obd_set_info_async(NULL, exp, strlen(KEY_CHANGELOG_CLEAR),
drivers/staging/lustre/lustre/obdclass/genops.c:624:		rc2 = obd_set_info_async(NULL, obd->obd_self_export,
drivers/staging/lustre/lustre/obdclass/obd_mount.c:277:		rc = obd_set_info_async(NULL, obd->obd_self_export,
drivers/staging/lustre/lustre/obdclass/obd_mount.c:326:		rc = obd_set_info_async(NULL, obd->obd_self_export,
drivers/staging/lustre/lustre/obdclass/obd_mount.c:425:	rc = obd_set_info_async(NULL, obd->obd_self_export,
drivers/staging/lustre/lustre/obdclass/obd_mount.c:437:	rc = obd_set_info_async(NULL, obd->obd_self_export,
;

Oh, joy... 23 call sites to sift through.  Which ones are we interested in?
The test down in the FPOS that has started it all is also reviewer-hostile -
KEY_IS(KEY_SET_FS) has no visible references to function arguments.  Oh, well...

#define KEY_IS(str) \
	(keylen >= (sizeof(str)-1) && memcmp(key, str, (sizeof(str)-1)) == 0)
#define KEY_SET_FS		"set_fs"

and KEY_SET_FS is _NOT_ mentioned anywhere outside that check.  Neither is
"set_fs" as explicit string constant.  I would've assumed the whole thing
to be dead code, but... what with the preprocessor tricks we'd already seen
there, I wouldn't bet against that thing coming from string concat.  Or from
macro stringifying set_fs (sans double quotes).  Or from any number of sick
preprocessor tricks.  Oh, well - at least we can go through that list and
drop the call sites that pass a different string constant.  A few minutes
and curses later we are down to these 5:

                        err = obd_set_info_async(env, tgt->ltd_exp,
                                                 keylen, key, vallen, val, set);
in lmv_set_info_async(),
and
			err = obd_set_info_async(env, tgt->ltd_exp,
					 keylen, key, sizeof(int),
					 &mgi->group, set);
			err = obd_set_info_async(env, tgt->ltd_exp,
					 keylen, key, vallen,
					 ((struct obd_id_info*)val)->data, set);
			err = obd_set_info_async(env, tgt->ltd_exp, keylen,
						 key, sizeof(*info->capa),
						 info->capa, set);
			err = obd_set_info_async(env, tgt->ltd_exp,
					 keylen, key, vallen, val, set);
in lov_set_info_async().

; git grep -n -w lmv_set_info_async
drivers/staging/lustre/lustre/lmv/lmv_obd.c:2212:int lmv_set_info_async(const struct lu_env *env, struct obd_export *exp,
drivers/staging/lustre/lustre/lmv/lmv_obd.c:2661:	.o_set_info_async       = lmv_set_info_async,
;

IOW, this one is another o_set_info_async instance and key/keylen/val/vallen
is passed from its caller as-is.

; git grep -n -w lov_set_info_async
drivers/staging/lustre/lustre/lov/lov_obd.c:2559:static int lov_set_info_async(const struct lu_env *env, struct obd_export *exp,
drivers/staging/lustre/lustre/lov/lov_obd.c:2850:	.o_set_info_async      = lov_set_info_async,
;

... and this one is also o_set_info_async instance.  In one of 4 call sites
we appear to pass the arguments as-is, the rest... key/keylen is passed as-is,
val/vallen isn't.  In any case, key must have originated in one of the
call sites we'd already eliminated.

So unless I've missed something in that paragon of good taste, there is no
call chain that could've lead to mgc_set_info_async() with key being "set_fs".
Incidentally, there is a couple of recursive loops in there that might or
might not lead to stack overflows.

Could somebody familiar with that thing confirm that this is, in fact, dead
code?  As the matter of fact, could maintainers please stand up and put their
names into MAINTAINERS?

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

[Peng Tao]

Hi Al,

On Thu, Jul 18, 2013 at 5:08 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
> [nathan cc'd since he's the only person mentioned in mgc_request.c]
>
clusterfs is long gone. Add nathan's new mail address. Also add
Andreas Dilger. He is the maintainer of the Lustre code.

> One general observation: drivers/taging/lustre is highly reviewer-hostile...
>
sorry...

> Found by unrelated grep:
>
> drivers/staging/lustre/lustre/mgc/mgc_request.c:1040:           if (vallen != sizeof(struct super_block))
>
> Huh?  The code around that line looks so:
>         if (KEY_IS(KEY_SET_FS)) {
>                 struct super_block *sb = (struct super_block *)val;
>                 struct lustre_sb_info *lsi;
>                 if (vallen != sizeof(struct super_block))
>                         RETURN(-EINVAL);
>                 lsi = s2lsi(sb);
>                 rc = mgc_fs_setup(exp->exp_obd, sb, lsi->lsi_srv_mnt);
>                 if (rc) {
>                         CERROR("set_fs got %d\n", rc);
>                 }
>                 RETURN(rc);
>         }
> What is going on here?  We cast something to struct super_block *?
> Where does it come from?  The function it's in is
>
> int mgc_set_info_async(const struct lu_env *env, struct obd_export *exp,
>                        obd_count keylen, void *key, obd_count vallen,
>                        void *val, struct ptlrpc_request_set *set)
>
> which says damn little, other than "multiplexor with no type safety".  Who
> calls that?
> ; git grep -n -w mgc_set_info_async
> drivers/staging/lustre/lustre/mgc/mgc_request.c:1001:int mgc_set_info_async(const struct lu_env *env, struct obd_export *exp,
> drivers/staging/lustre/lustre/mgc/mgc_request.c:1836:   .o_set_info_async = mgc_set_info_async,
> ;
>
> Umm...  OK, looks like it's only called as a method.  Unfortunately, grep for
> o_set_info_async brings a lot of instances and no callers.  After a lot of
> cursing, the following antisocial Fine Piece Of Code had been located:
>
> #define OBP(dev, op)    (dev)->obd_type->typ_dt_ops->o_ ## op
>
> along with
>
>         rc = OBP(exp->exp_obd, set_info_async)(env, exp, keylen, key, vallen,
>                                                val, set);
>
> As far as I'm concerned, macros like that are equivalent to spitting into
> reviewers' eyes.  Sometimes out of malice, sometimes just because the authors
> felt like it would be a cute prank, either way it makes life really unpleasant -
> when you need to guess which part of method/function name is to be searched for
> to locate the call sites... it gets old very soon.
It caused a lot of trouble for many to follow the code...

Andreas, can we just drop such macros?

> Anyway, this thing is in
>
> static inline int obd_set_info_async(const struct lu_env *env,
>                                      struct obd_export *exp, obd_count keylen,
>                                      void *key, obd_count vallen, void *val,
>                                      struct ptlrpc_request_set *set)
>
> Again, the type safety is inexistent, but let's cross the fingers and look for
> callers (and hope they hadn't pulled a similar cute trick with ## here)
>
> ; git grep -n -w obd_set_info_async
> drivers/staging/lustre/lustre/include/obd_class.h:508:static inline int obd_set_info_async(const struct lu_env *env,
> drivers/staging/lustre/lustre/ldlm/ldlm_lockd.c:522:            rc = obd_set_info_async(req->rq_svc_thread->t_env,
> drivers/staging/lustre/lustre/llite/dir.c:658:  rc = obd_set_info_async(NULL, mgc, sizeof(KEY_SET_INFO), KEY_SET_INFO,
> drivers/staging/lustre/lustre/llite/llite_lib.c:558:    err = obd_set_info_async(NULL, sbi->ll_dt_exp, sizeof(KEY_CHECKSUM),
> drivers/staging/lustre/lustre/llite/llite_lib.c:563:    err = obd_set_info_async(NULL, sbi->ll_dt_exp, sizeof(KEY_CACHE_SET),
> drivers/staging/lustre/lustre/llite/llite_lib.c:1964:   obd_set_info_async(NULL, sbi->ll_md_exp,
> drivers/staging/lustre/lustre/llite/llite_lib.c:1967:   obd_set_info_async(NULL, sbi->ll_dt_exp,
> drivers/staging/lustre/lustre/llite/llite_lib.c:2032:           err = obd_set_info_async(NULL, sbi->ll_md_exp,
> drivers/staging/lustre/lustre/llite/lproc_llite.c:437:          rc = obd_set_info_async(NULL, sbi->ll_dt_exp,
> drivers/staging/lustre/lustre/llite/lproc_llite.c:485:  rc = obd_set_info_async(NULL, sbi->ll_dt_exp, sizeof(KEY_CHECKSUM),
> drivers/staging/lustre/lustre/lmv/lmv_obd.c:281:                obd_set_info_async(NULL, tgt->ltd_exp, sizeof(KEY_INTERMDS),
> drivers/staging/lustre/lustre/lmv/lmv_obd.c:2239:                       err = obd_set_info_async(env, tgt->ltd_exp,
> drivers/staging/lustre/lustre/lov/lov_obd.c:638:                rc = obd_set_info_async(NULL, tgt->ltd_exp,
> drivers/staging/lustre/lustre/lov/lov_obd.c:2629:                       err = obd_set_info_async(env, tgt->ltd_exp,
> drivers/staging/lustre/lustre/lov/lov_obd.c:2633:                       err = obd_set_info_async(env, tgt->ltd_exp,
> drivers/staging/lustre/lustre/lov/lov_obd.c:2646:                       err = obd_set_info_async(env, tgt->ltd_exp, keylen,
> drivers/staging/lustre/lustre/lov/lov_obd.c:2655:                       err = obd_set_info_async(env, tgt->ltd_exp,
> drivers/staging/lustre/lustre/mdc/mdc_request.c:1767:           rc = obd_set_info_async(NULL, exp, strlen(KEY_CHANGELOG_CLEAR),
> drivers/staging/lustre/lustre/obdclass/genops.c:624:            rc2 = obd_set_info_async(NULL, obd->obd_self_export,
> drivers/staging/lustre/lustre/obdclass/obd_mount.c:277:         rc = obd_set_info_async(NULL, obd->obd_self_export,
> drivers/staging/lustre/lustre/obdclass/obd_mount.c:326:         rc = obd_set_info_async(NULL, obd->obd_self_export,
> drivers/staging/lustre/lustre/obdclass/obd_mount.c:425: rc = obd_set_info_async(NULL, obd->obd_self_export,
> drivers/staging/lustre/lustre/obdclass/obd_mount.c:437: rc = obd_set_info_async(NULL, obd->obd_self_export,
> ;
>
> Oh, joy... 23 call sites to sift through.  Which ones are we interested in?
> The test down in the FPOS that has started it all is also reviewer-hostile -
> KEY_IS(KEY_SET_FS) has no visible references to function arguments.  Oh, well...
>
> #define KEY_IS(str) \
>         (keylen >= (sizeof(str)-1) && memcmp(key, str, (sizeof(str)-1)) == 0)
> #define KEY_SET_FS              "set_fs"
>
> and KEY_SET_FS is _NOT_ mentioned anywhere outside that check.  Neither is
> "set_fs" as explicit string constant.  I would've assumed the whole thing
> to be dead code, but... what with the preprocessor tricks we'd already seen
> there, I wouldn't bet against that thing coming from string concat.  Or from
> macro stringifying set_fs (sans double quotes).  Or from any number of sick
> preprocessor tricks.  Oh, well - at least we can go through that list and
> drop the call sites that pass a different string constant.  A few minutes
> and curses later we are down to these 5:
>
>                         err = obd_set_info_async(env, tgt->ltd_exp,
>                                                  keylen, key, vallen, val, set);
> in lmv_set_info_async(),
> and
>                         err = obd_set_info_async(env, tgt->ltd_exp,
>                                          keylen, key, sizeof(int),
>                                          &mgi->group, set);
>                         err = obd_set_info_async(env, tgt->ltd_exp,
>                                          keylen, key, vallen,
>                                          ((struct obd_id_info*)val)->data, set);
>                         err = obd_set_info_async(env, tgt->ltd_exp, keylen,
>                                                  key, sizeof(*info->capa),
>                                                  info->capa, set);
>                         err = obd_set_info_async(env, tgt->ltd_exp,
>                                          keylen, key, vallen, val, set);
> in lov_set_info_async().
>
> ; git grep -n -w lmv_set_info_async
> drivers/staging/lustre/lustre/lmv/lmv_obd.c:2212:int lmv_set_info_async(const struct lu_env *env, struct obd_export *exp,
> drivers/staging/lustre/lustre/lmv/lmv_obd.c:2661:       .o_set_info_async       = lmv_set_info_async,
> ;
>
> IOW, this one is another o_set_info_async instance and key/keylen/val/vallen
> is passed from its caller as-is.
>
> ; git grep -n -w lov_set_info_async
> drivers/staging/lustre/lustre/lov/lov_obd.c:2559:static int lov_set_info_async(const struct lu_env *env, struct obd_export *exp,
> drivers/staging/lustre/lustre/lov/lov_obd.c:2850:       .o_set_info_async      = lov_set_info_async,
> ;
>
> ... and this one is also o_set_info_async instance.  In one of 4 call sites
> we appear to pass the arguments as-is, the rest... key/keylen is passed as-is,
> val/vallen isn't.  In any case, key must have originated in one of the
> call sites we'd already eliminated.
>
> So unless I've missed something in that paragon of good taste, there is no
> call chain that could've lead to mgc_set_info_async() with key being "set_fs".
> Incidentally, there is a couple of recursive loops in there that might or
> might not lead to stack overflows.
>
You analysis looks correct. After searching down the list on my own, I
agree with you that the piece of code is unreachable at all. But it is
better to get confirmed by Andreas or Nathan.

> Could somebody familiar with that thing confirm that this is, in fact, dead
> code?  As the matter of fact, could maintainers please stand up and put their
> names into MAINTAINERS?

Andreas, please?

Thanks,
Tao

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

[Al Viro]

On Thu, Jul 18, 2013 at 11:40:16AM -0700, Nathan Rutman wrote:
> >>                }
> >>                RETURN(rc);
> >>        }
> >> What is going on here?  We cast something to struct super_block *?
> >> Where does it come from?  The function it's in is
> Well, addressing the "what's going on" question without getting into the larger philosophy,
> keys and values are used as a generic mechanism to pass various items between Lustre clients
> and servers.  In this case, a specific key should only have a value of "a superblock", and so this is 
> just a sanity check to make sure the value length is sane.  It should probably be more of an ASSERT,
> but we can't reasonably assert on remotely-supplied data.

What?  Excuse me, but have you seriously been intending to pass struct
super_block instances around?  Ones that are choke-full of pointers to
all kinds of things, not to mention a mutex, spinlock, etc.?

_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().

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

[Andreas Dilger]

On 2013-07-18, at 1:07 PM, Al Viro wrote:
> On Thu, Jul 18, 2013 at 11:40:16AM -0700, Nathan Rutman wrote:
>>>>               }
>>>>               RETURN(rc);
>>>>       }
>>>> What is going on here?  We cast something to struct super_block *?
>>>> Where does it come from?  The function it's in is
>> Well, addressing the "what's going on" question without getting
>> into the larger philosophy, keys and values are used as a generic mechanism to pass various items between Lustre clients and servers.
>> In this case, a specific key should only have a value of "a
>> superblock", and so this is just a sanity check to make sure the
>> value length is sane.  It should probably be more of an ASSERT,
>> but we can't reasonably assert on remotely-supplied data.
> 
> What?  Excuse me, but have you seriously been intending to pass
> struct super_block instances around?  Ones that are choke-full of
> pointers to all kinds of things, not to mention a mutex, spinlock,
> etc.?

The set_info() code is also used between modules on the same node.
I don't make any excuses for the cleanliness of this code, which is
why it is in staging and will take time to clean up before even
attempting to get it into fs/.  I apologize that you even had to
look at this code - definitely a twisty maze that needs cleaning.

> _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.

Cheers, Andreas

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

[Al Viro]

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...

I honestly have no idea about your plans wrt merging; are you going
to put the server side of things there as well?

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.

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.

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

[Peng, Tao]

On Fri, Jul 19, 2013 at 4:12 PM, Al Viro <viro@zeniv.linux.org.uk> 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