Re: [PATCH] staging: lustre: ptlrpc: re-export lustre_swab_[lmv|lov]_mds_md
> On Mon, Sep 19, 2016 at 01:27:05PM -0400, James Simmons wrote: > > Being over zealous in removing unused EXPORT_SYMBOLs two functions > > lustre_swab_[lmv|lov]_mds_md exports were removed. They need to be > > exported so this patch restores those EXPORT_SYMBOLS. Same mistake > > was done when porting to the upstream client. > > > > Signed-off-by: James Simmons > > Reviewed-on: http://review.whamcloud.com/14545 > > Reviewed-on: http://review.whamcloud.com/15159 > > Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-6486 > > Reviewed-by: Dmitry Eremin > > Reviewed-by: Bob Glossman > > Reviewed-by: John L. Hammond > > Reviewed-by: Oleg Drokin > > Signed-off-by: James Simmons > > --- > > .../staging/lustre/lustre/ptlrpc/pack_generic.c|2 ++ > > 1 files changed, 2 insertions(+), 0 deletions(-) > > > > diff --git a/drivers/staging/lustre/lustre/ptlrpc/pack_generic.c > > b/drivers/staging/lustre/lustre/ptlrpc/pack_generic.c > > index 1349bf6..8717685 100644 > > --- a/drivers/staging/lustre/lustre/ptlrpc/pack_generic.c > > +++ b/drivers/staging/lustre/lustre/ptlrpc/pack_generic.c > > @@ -1861,6 +1861,7 @@ void lustre_swab_lmv_mds_md(union lmv_mds_md *lmm) > > break; > > } > > } > > +EXPORT_SYMBOL(lustre_swab_lmv_mds_md); > > I don't see anyone else using this symbol, in fact, it could now be > marked static. So why export it? Too soon to export it. A patch is coming in which the llite layer will use it. I will drop it and make a note to make sure for the upcoming patch that I export lustre_swab_lmv_mds_md. > > void lustre_swab_lmv_user_md(struct lmv_user_md *lum) > > { > > @@ -1914,6 +1915,7 @@ void lustre_swab_lov_mds_md(struct lov_mds_md *lmm) > > __swab16s(&lmm->lmm_stripe_count); > > __swab16s(&lmm->lmm_layout_gen); > > } > > +EXPORT_SYMBOL(lustre_swab_lov_mds_md); > > This is used by other files (it's listed twice in lustre_idl.h...), so > it might need to be exported, but why am I not seeing a build error > without this change? The function lustre_swab_lov_mds_md is also used in lov_pack.c so it needs to be exported. You shouldn't be able to load some of the lustre modules but it does on x86. I will send a new patch to fix this but a note about this breakage should be kept so someone with an indepth understanding of the module system can track down what is going wrong.
Re: [lustre-devel] [PATCH] staging: lustre: ptlrpc: re-export lustre_swab_[lmv|lov]_mds_md
> > On Tue, Sep 20, 2016 at 11:52:19AM +0300, Dan Carpenter wrote: > > > On Tue, Sep 20, 2016 at 08:47:02AM +0200, Greg Kroah-Hartman wrote: > > > > On Mon, Sep 19, 2016 at 01:27:05PM -0400, James Simmons wrote: > > > > > Being over zealous in removing unused EXPORT_SYMBOLs two functions > > > > > lustre_swab_[lmv|lov]_mds_md exports were removed. They need to be > > > > > exported so this patch restores those EXPORT_SYMBOLS. Same mistake > > > > > was done when porting to the upstream client. > > > > > > > > How did our build testing not catch this? What needs these exports? Is > > > > it any in-kernel code? > > > > > > > > confused, > > > > > > It did catch it... > > > > It did? Works here for me, and I didn't see an error report anywhere... > > > > > James, every patch has to be buildable (bisectable) so the original > > > patch is never going to be merged. > > > > What is the "original" patch here? > > > > [PATCH 112/124] staging: lustre: ptlrpc: remove unnecessary EXPORT_SYMBOL > > kbuild responded to the email that it broke compilation on s360 but I > don't think the breakage is arch specific? But it is. The breakage appears on only a subset of platforms. All the patches I sent are tested on a real file system setup using our special test suite. This was done on x86 hardware where the problem doesn't show up. I guess I need to ask work for a non-x86 platform to test with :-)
Re: [PATCH] staging: lustre: ptlrpc: re-export lustre_swab_[lmv|lov]_mds_md
> > On Mon, Sep 19, 2016 at 01:27:05PM -0400, James Simmons wrote: > > > Being over zealous in removing unused EXPORT_SYMBOLs two functions > > > lustre_swab_[lmv|lov]_mds_md exports were removed. They need to be > > > exported so this patch restores those EXPORT_SYMBOLS. Same mistake > > > was done when porting to the upstream client. > > > > How did our build testing not catch this? What needs these exports? Is > > it any in-kernel code? > > > > confused, > > It did catch it... James, every patch has to be buildable (bisectable) > so the original patch is never going to be merged. We missed it original as well due to the fact it showed up on only specific platforms. I have no idea why that is. The majority of the time it works. BTW is there a way to run kbuild bot on a patch set before submission to avoid these corner cases.
Re: [PATCH] staging: lustre: ptlrpc: re-export lustre_swab_[lmv|lov]_mds_md
On Tue, Sep 20, 2016 at 02:16:12PM +0300, Dan Carpenter wrote: > On Tue, Sep 20, 2016 at 01:05:00PM +0200, Greg Kroah-Hartman wrote: > > On Tue, Sep 20, 2016 at 11:52:19AM +0300, Dan Carpenter wrote: > > > On Tue, Sep 20, 2016 at 08:47:02AM +0200, Greg Kroah-Hartman wrote: > > > > On Mon, Sep 19, 2016 at 01:27:05PM -0400, James Simmons wrote: > > > > > Being over zealous in removing unused EXPORT_SYMBOLs two functions > > > > > lustre_swab_[lmv|lov]_mds_md exports were removed. They need to be > > > > > exported so this patch restores those EXPORT_SYMBOLS. Same mistake > > > > > was done when porting to the upstream client. > > > > > > > > How did our build testing not catch this? What needs these exports? Is > > > > it any in-kernel code? > > > > > > > > confused, > > > > > > It did catch it... > > > > It did? Works here for me, and I didn't see an error report anywhere... > > > > > James, every patch has to be buildable (bisectable) so the original > > > patch is never going to be merged. > > > > What is the "original" patch here? > > > > [PATCH 112/124] staging: lustre: ptlrpc: remove unnecessary EXPORT_SYMBOL > > kbuild responded to the email that it broke compilation on s360 but I > don't think the breakage is arch specific? Crap, I thought that was some other kbuild failure, my fault for ignoring that. Ok, I'll take half of this patch, if it gets resent... thanks, greg k-h
Re: [PATCH] staging: lustre: ptlrpc: re-export lustre_swab_[lmv|lov]_mds_md
On Mon, Sep 19, 2016 at 01:27:05PM -0400, James Simmons wrote: > Being over zealous in removing unused EXPORT_SYMBOLs two functions > lustre_swab_[lmv|lov]_mds_md exports were removed. They need to be > exported so this patch restores those EXPORT_SYMBOLS. Same mistake > was done when porting to the upstream client. > > Signed-off-by: James Simmons > Reviewed-on: http://review.whamcloud.com/14545 > Reviewed-on: http://review.whamcloud.com/15159 > Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-6486 > Reviewed-by: Dmitry Eremin > Reviewed-by: Bob Glossman > Reviewed-by: John L. Hammond > Reviewed-by: Oleg Drokin > Signed-off-by: James Simmons > --- > .../staging/lustre/lustre/ptlrpc/pack_generic.c|2 ++ > 1 files changed, 2 insertions(+), 0 deletions(-) > > diff --git a/drivers/staging/lustre/lustre/ptlrpc/pack_generic.c > b/drivers/staging/lustre/lustre/ptlrpc/pack_generic.c > index 1349bf6..8717685 100644 > --- a/drivers/staging/lustre/lustre/ptlrpc/pack_generic.c > +++ b/drivers/staging/lustre/lustre/ptlrpc/pack_generic.c > @@ -1861,6 +1861,7 @@ void lustre_swab_lmv_mds_md(union lmv_mds_md *lmm) > break; > } > } > +EXPORT_SYMBOL(lustre_swab_lmv_mds_md); I don't see anyone else using this symbol, in fact, it could now be marked static. So why export it? > void lustre_swab_lmv_user_md(struct lmv_user_md *lum) > { > @@ -1914,6 +1915,7 @@ void lustre_swab_lov_mds_md(struct lov_mds_md *lmm) > __swab16s(&lmm->lmm_stripe_count); > __swab16s(&lmm->lmm_layout_gen); > } > +EXPORT_SYMBOL(lustre_swab_lov_mds_md); This is used by other files (it's listed twice in lustre_idl.h...), so it might need to be exported, but why am I not seeing a build error without this change? confused, greg k-h
Re: [PATCH] staging: lustre: ptlrpc: re-export lustre_swab_[lmv|lov]_mds_md
On Tue, Sep 20, 2016 at 11:52:19AM +0300, Dan Carpenter wrote: > On Tue, Sep 20, 2016 at 08:47:02AM +0200, Greg Kroah-Hartman wrote: > > On Mon, Sep 19, 2016 at 01:27:05PM -0400, James Simmons wrote: > > > Being over zealous in removing unused EXPORT_SYMBOLs two functions > > > lustre_swab_[lmv|lov]_mds_md exports were removed. They need to be > > > exported so this patch restores those EXPORT_SYMBOLS. Same mistake > > > was done when porting to the upstream client. > > > > How did our build testing not catch this? What needs these exports? Is > > it any in-kernel code? > > > > confused, > > It did catch it... It did? Works here for me, and I didn't see an error report anywhere... > James, every patch has to be buildable (bisectable) so the original > patch is never going to be merged. What is the "original" patch here? totally confused, greg k-h
Re: [PATCH] staging: lustre: ptlrpc: re-export lustre_swab_[lmv|lov]_mds_md
On Tue, Sep 20, 2016 at 01:05:00PM +0200, Greg Kroah-Hartman wrote: > On Tue, Sep 20, 2016 at 11:52:19AM +0300, Dan Carpenter wrote: > > On Tue, Sep 20, 2016 at 08:47:02AM +0200, Greg Kroah-Hartman wrote: > > > On Mon, Sep 19, 2016 at 01:27:05PM -0400, James Simmons wrote: > > > > Being over zealous in removing unused EXPORT_SYMBOLs two functions > > > > lustre_swab_[lmv|lov]_mds_md exports were removed. They need to be > > > > exported so this patch restores those EXPORT_SYMBOLS. Same mistake > > > > was done when porting to the upstream client. > > > > > > How did our build testing not catch this? What needs these exports? Is > > > it any in-kernel code? > > > > > > confused, > > > > It did catch it... > > It did? Works here for me, and I didn't see an error report anywhere... > > > James, every patch has to be buildable (bisectable) so the original > > patch is never going to be merged. > > What is the "original" patch here? > [PATCH 112/124] staging: lustre: ptlrpc: remove unnecessary EXPORT_SYMBOL kbuild responded to the email that it broke compilation on s360 but I don't think the breakage is arch specific? regards, dan carpenter
Re: [PATCH] staging: lustre: ptlrpc: re-export lustre_swab_[lmv|lov]_mds_md
On Tue, Sep 20, 2016 at 08:47:02AM +0200, Greg Kroah-Hartman wrote: > On Mon, Sep 19, 2016 at 01:27:05PM -0400, James Simmons wrote: > > Being over zealous in removing unused EXPORT_SYMBOLs two functions > > lustre_swab_[lmv|lov]_mds_md exports were removed. They need to be > > exported so this patch restores those EXPORT_SYMBOLS. Same mistake > > was done when porting to the upstream client. > > How did our build testing not catch this? What needs these exports? Is > it any in-kernel code? > > confused, It did catch it... James, every patch has to be buildable (bisectable) so the original patch is never going to be merged. regards, dan carpenter
Re: [PATCH] staging: lustre: ptlrpc: re-export lustre_swab_[lmv|lov]_mds_md
On Mon, Sep 19, 2016 at 01:27:05PM -0400, James Simmons wrote: > Being over zealous in removing unused EXPORT_SYMBOLs two functions > lustre_swab_[lmv|lov]_mds_md exports were removed. They need to be > exported so this patch restores those EXPORT_SYMBOLS. Same mistake > was done when porting to the upstream client. How did our build testing not catch this? What needs these exports? Is it any in-kernel code? confused, greg k-h
[PATCH] staging: lustre: ptlrpc: re-export lustre_swab_[lmv|lov]_mds_md
Being over zealous in removing unused EXPORT_SYMBOLs two functions lustre_swab_[lmv|lov]_mds_md exports were removed. They need to be exported so this patch restores those EXPORT_SYMBOLS. Same mistake was done when porting to the upstream client. Signed-off-by: James Simmons Reviewed-on: http://review.whamcloud.com/14545 Reviewed-on: http://review.whamcloud.com/15159 Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-6486 Reviewed-by: Dmitry Eremin Reviewed-by: Bob Glossman Reviewed-by: John L. Hammond Reviewed-by: Oleg Drokin Signed-off-by: James Simmons --- .../staging/lustre/lustre/ptlrpc/pack_generic.c|2 ++ 1 files changed, 2 insertions(+), 0 deletions(-) diff --git a/drivers/staging/lustre/lustre/ptlrpc/pack_generic.c b/drivers/staging/lustre/lustre/ptlrpc/pack_generic.c index 1349bf6..8717685 100644 --- a/drivers/staging/lustre/lustre/ptlrpc/pack_generic.c +++ b/drivers/staging/lustre/lustre/ptlrpc/pack_generic.c @@ -1861,6 +1861,7 @@ void lustre_swab_lmv_mds_md(union lmv_mds_md *lmm) break; } } +EXPORT_SYMBOL(lustre_swab_lmv_mds_md); void lustre_swab_lmv_user_md(struct lmv_user_md *lum) { @@ -1914,6 +1915,7 @@ void lustre_swab_lov_mds_md(struct lov_mds_md *lmm) __swab16s(&lmm->lmm_stripe_count); __swab16s(&lmm->lmm_layout_gen); } +EXPORT_SYMBOL(lustre_swab_lov_mds_md); void lustre_swab_lov_user_md_objects(struct lov_user_ost_data *lod, int stripe_count) -- 1.7.1