[PATCH] libosd: Remove ignored __weak attribute

[PATCH] libosd: Remove ignored __weak attribute

[Nathan Chancellor]

Clang warns that the __weak attribute is going to be ignored on
osd_root_object because it's not in the correct location (needs to be
after the type).

./include/scsi/osd_types.h:31:21: warning: 'weak' attribute only applies
to variables, functions, and classes [-Wignored-attributes]
static const struct __weak osd_obj_id osd_root_object = {0, 0};
                    ^

Turns out that GCC ignores the attribute too albeit silently because
moving the attribute after either osd_obj_id or osd_root_object like
all other uses of __weak on variables in the kernel causes a build
error on both GCC and Clang because static variables cannot be weak
since weak definitions rely on not having internal linkage:

./include/scsi/osd_types.h:31:32: error: weak declaration cannot have
internal linkage
static const struct osd_obj_id __weak osd_root_object = {0, 0};
                               ^

Just remove the attribute because it hasn't been correct since the
initial addition of this file in commit de258bf5e638 ("[SCSI] libosd:
OSDv1 Headers").

Reported-by: Nick Desaulniers <ndesaulniers@google.com>
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
 include/scsi/osd_types.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/scsi/osd_types.h b/include/scsi/osd_types.h
index 48e8a165e136..6b6fdcafa6cc 100644
--- a/include/scsi/osd_types.h
+++ b/include/scsi/osd_types.h
@@ -28,7 +28,7 @@ struct osd_obj_id {
 	osd_id id;
 };
 
-static const struct __weak osd_obj_id osd_root_object = {0, 0};
+static const struct osd_obj_id osd_root_object = {0, 0};
 
 struct osd_attr {
 	u32 attr_page;
-- 
2.19.0

Re: [PATCH] libosd: Remove ignored __weak attribute

[Nick Desaulniers]

On Sun, Sep 30, 2018 at 1:55 PM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> Clang warns that the __weak attribute is going to be ignored on
> osd_root_object because it's not in the correct location (needs to be
> after the type).
>
> ./include/scsi/osd_types.h:31:21: warning: 'weak' attribute only applies
> to variables, functions, and classes [-Wignored-attributes]
> static const struct __weak osd_obj_id osd_root_object = {0, 0};
>                     ^
>
> Turns out that GCC ignores the attribute too albeit silently because
> moving the attribute after either osd_obj_id or osd_root_object like
> all other uses of __weak on variables in the kernel causes a build
> error on both GCC and Clang because static variables cannot be weak
> since weak definitions rely on not having internal linkage:
>
> ./include/scsi/osd_types.h:31:32: error: weak declaration cannot have
> internal linkage
> static const struct osd_obj_id __weak osd_root_object = {0, 0};
>                                ^
>
> Just remove the attribute because it hasn't been correct since the
> initial addition of this file in commit de258bf5e638 ("[SCSI] libosd:
> OSDv1 Headers").
>
> Reported-by: Nick Desaulniers <ndesaulniers@google.com>
> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
> ---
>  include/scsi/osd_types.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/scsi/osd_types.h b/include/scsi/osd_types.h
> index 48e8a165e136..6b6fdcafa6cc 100644
> --- a/include/scsi/osd_types.h
> +++ b/include/scsi/osd_types.h
> @@ -28,7 +28,7 @@ struct osd_obj_id {
>         osd_id id;
>  };
>
> -static const struct __weak osd_obj_id osd_root_object = {0, 0};
> +static const struct osd_obj_id osd_root_object = {0, 0};
>
>  struct osd_attr {
>         u32 attr_page;
> --
> 2.19.0
>

LGTM, thank you for sending, Nathan.
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] libosd: Remove ignored __weak attribute

[Bart Van Assche]

On 9/30/18 1:54 PM, Nathan Chancellor wrote:
> diff --git a/include/scsi/osd_types.h b/include/scsi/osd_types.h
> index 48e8a165e136..6b6fdcafa6cc 100644
> --- a/include/scsi/osd_types.h
> +++ b/include/scsi/osd_types.h
> @@ -28,7 +28,7 @@ struct osd_obj_id {
>   	osd_id id;
>   };
>   
> -static const struct __weak osd_obj_id osd_root_object = {0, 0};
> +static const struct osd_obj_id osd_root_object = {0, 0};

Structure definitions should occur in .c files instead of in header 
files especially if the header file is included from multiple source 
files. Please consider moving the definition of osd_root_object into a 
.c file. Additionally, zero initializers should be left out to minimize 
the size of object files.

Boaz, the most recent osd patch that is neither trivial nor treewide 
refactoring is six years old (51976a8c85ce ("[SCSI] osd_uld: Add osdname 
& systemid sysfs at scsi_osd class"). That suggests that nobody is using 
this driver anymore. Can this driver be removed from the kernel tree?

Thanks,

Bart.

Re: [PATCH] libosd: Remove ignored __weak attribute

[Nathan Chancellor]

On Mon, Oct 01, 2018 at 06:16:32PM -0700, Bart Van Assche wrote:
> On 9/30/18 1:54 PM, Nathan Chancellor wrote:
> > diff --git a/include/scsi/osd_types.h b/include/scsi/osd_types.h
> > index 48e8a165e136..6b6fdcafa6cc 100644
> > --- a/include/scsi/osd_types.h
> > +++ b/include/scsi/osd_types.h
> > @@ -28,7 +28,7 @@ struct osd_obj_id {
> >   	osd_id id;
> >   };
> > -static const struct __weak osd_obj_id osd_root_object = {0, 0};
> > +static const struct osd_obj_id osd_root_object = {0, 0};
> 

Hi Bart,

> Structure definitions should occur in .c files instead of in header files
> especially if the header file is included from multiple source files. Please
> consider moving the definition of osd_root_object into a .c file.
> Additionally, zero initializers should be left out to minimize the size of
> object files.
> 

I'm perfectly happy to make this change in a v2 if necessary.

> Boaz, the most recent osd patch that is neither trivial nor treewide
> refactoring is six years old (51976a8c85ce ("[SCSI] osd_uld: Add osdname &
> systemid sysfs at scsi_osd class"). That suggests that nobody is using this
> driver anymore. Can this driver be removed from the kernel tree?
> 

However, this is certainly a better option if possible.

> Thanks,
> 
> Bart.

Thanks for the comments!
Nathan

Re: [PATCH] libosd: Remove ignored __weak attribute

[Christoph Hellwig]

On Mon, Oct 01, 2018 at 06:16:32PM -0700, Bart Van Assche wrote:
> Boaz, the most recent osd patch that is neither trivial nor treewide
> refactoring is six years old (51976a8c85ce ("[SCSI] osd_uld: Add osdname &
> systemid sysfs at scsi_osd class"). That suggests that nobody is using this
> driver anymore. Can this driver be removed from the kernel tree?

Last time we tried to exofs and the osd code Boaz complained loudly,
but as far as I can tell it really is not used and bitrotting, so we
should finally go ahead and drop it.

Re: [PATCH] libosd: Remove ignored __weak attribute

[Boaz Harrosh]

On 02/10/18 17:56, Christoph Hellwig wrote:
> On Mon, Oct 01, 2018 at 06:16:32PM -0700, Bart Van Assche wrote:
>> Boaz, the most recent osd patch that is neither trivial nor treewide
>> refactoring is six years old (51976a8c85ce ("[SCSI] osd_uld: Add osdname &
>> systemid sysfs at scsi_osd class"). That suggests that nobody is using this
>> driver anymore. Can this driver be removed from the kernel tree?
> 
> Last time we tried to exofs and the osd code Boaz complained loudly,
> but as far as I can tell it really is not used and bitrotting, so we
> should finally go ahead and drop it.
> 

As I said then. It is used in Universities for studies and experiments.
Every once in a while. I get an email with questions and reports.

But yes feel free to remove the all thing!!

I guess I can put it up on github. In a public tree.

Just that I will need to forward port it myself, til now you guys
been doing this for me ;-)

Thanks, sorry for the hassle
Boaz

Re: [PATCH] libosd: Remove ignored __weak attribute

[Nick Desaulniers]

On Mon, Oct 1, 2018 at 6:16 PM Bart Van Assche <bvanassche@acm.org> wrote:
>
> On 9/30/18 1:54 PM, Nathan Chancellor wrote:
> > diff --git a/include/scsi/osd_types.h b/include/scsi/osd_types.h
> > index 48e8a165e136..6b6fdcafa6cc 100644
> > --- a/include/scsi/osd_types.h
> > +++ b/include/scsi/osd_types.h
> > @@ -28,7 +28,7 @@ struct osd_obj_id {
> >       osd_id id;
> >   };
> >
> > -static const struct __weak osd_obj_id osd_root_object = {0, 0};
> > +static const struct osd_obj_id osd_root_object = {0, 0};
>
> Structure definitions should occur in .c files instead of in header
> files especially if the header file is included from multiple source
> files. Please consider moving the definition of osd_root_object into a
> .c file.

> Additionally, zero initializers should be left out to minimize
> the size of object files.

Sorry, my understanding was that global variables either occupy the
.bss section or the .data section, depending on whether they were
zero-initialized vs initialized to non-zero, respectively (where
non-initialized are treated as zero initialized).  Looks like without
the explicit zero initialization, compilers will put the symbols in a
"common" section, which `man 1 nm` says is also unitialized data.  I
didn't think .bss sections occupied space in an object file or binary;
the kernel's loader would set up the mappings at execution?  Can you
clarify?

>
> Boaz, the most recent osd patch that is neither trivial nor treewide
> refactoring is six years old (51976a8c85ce ("[SCSI] osd_uld: Add osdname
> & systemid sysfs at scsi_osd class"). That suggests that nobody is using
> this driver anymore. Can this driver be removed from the kernel tree?
>
> Thanks,
>
> Bart.



-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] libosd: Remove ignored __weak attribute

[Bart Van Assche]

On Tue, 2018-10-02 at 10:24 -0700, Nick Desaulniers wrote:
> On Mon, Oct 1, 2018 at 6:16 PM Bart Van Assche <bvanassche@acm.org> wrote:
> > Additionally, zero initializers should be left out to minimize
> > the size of object files.
> 
> Sorry, my understanding was that global variables either occupy the
> .bss section or the .data section, depending on whether they were
> zero-initialized vs initialized to non-zero, respectively (where
> non-initialized are treated as zero initialized).  Looks like without
> the explicit zero initialization, compilers will put the symbols in a
> "common" section, which `man 1 nm` says is also unitialized data.  I
> didn't think .bss sections occupied space in an object file or binary;
> the kernel's loader would set up the mappings at execution?  Can you
> clarify?

Explicitly initialized global and static variables end up in the .data
section and need space in that section. That is not the case if the
initializer is left out and these variables end up in the .bss section.
From https://en.wikipedia.org/wiki/.bss:

"In computer programming, the name .bss or bss is used by many compilers
and linkers for the portion of an object file or executable containing
statically-allocated variables that are not explicitly initialized to any
value. It is often referred to as the "bss section" or "bss segment".

Typically only the length of the bss section, but no data, is stored in
the object file."

This is why checkpatch warns if a global or static variable is initialized
explicitly to zero. From scripts/checkpatch.pl:

our $zero_initializer = qr{(?:(?:0[xX])?0+$Int_type?|NULL|false)\b};

# check for global initialisers.

                if ($line =~ /^\+$Type\s*$Ident(?:\s+$Modifier)*\s*=\s*($zero_initializer)\s*;/) {
                        if (ERROR("GLOBAL_INITIALISERS",
                                  "do not initialise globals to $1\n" . $herecurr) && $fix) {
                                $fixed[$fixlinenr] =~ s/(^.$Type\s*$Ident(?:\s+$Modifier)*)\s*=\s*$zero_initializer\s*;/$1;/;
                        }
                }

# check for static initialisers.

                if ($line =~ /^\+.*\bstatic\s.*=\s*($zero_initializer)\s*;/) {
                        if (ERROR("INITIALISED_STATIC",
                                  "do not initialise statics to $1\n" . $herecurr) && $fix) {
                                $fixed[$fixlinenr] =~ s/(\bstatic\s.*?)\s*=\s*$zero_initializer\s*;/$1;/;
                        }
                }

Bart.

Re: [PATCH] libosd: Remove ignored __weak attribute

[Nick Desaulniers]

On Tue, Oct 2, 2018 at 10:57 AM Bart Van Assche <bvanassche@acm.org> wrote:
>
> On Tue, 2018-10-02 at 10:24 -0700, Nick Desaulniers wrote:
> > On Mon, Oct 1, 2018 at 6:16 PM Bart Van Assche <bvanassche@acm.org> wrote:
> > > Additionally, zero initializers should be left out to minimize
> > > the size of object files.
> >
> > Sorry, my understanding was that global variables either occupy the
> > .bss section or the .data section, depending on whether they were
> > zero-initialized vs initialized to non-zero, respectively (where
> > non-initialized are treated as zero initialized).  Looks like without
> > the explicit zero initialization, compilers will put the symbols in a
> > "common" section, which `man 1 nm` says is also unitialized data.  I
> > didn't think .bss sections occupied space in an object file or binary;
> > the kernel's loader would set up the mappings at execution?  Can you
> > clarify?
>
> Explicitly initialized global and static variables end up in the .data
> section and need space in that section.

Unless the initial value is zero.
https://godbolt.org/z/curRoO

So you don't wind up with an increase in binary size simply by having
global variables initialized to zero, right?  Instead the kernel knows
to create a zero'd out mapping for bss.  You don't need a run of zeros
in the binary.

So I disagree when you said earlier "zero initializers should be left
out to minimize the size of object files." I assert they don't affect
the size of the binary.

If you had many global variables all initialized to zero, why would
you encode that many zeros in a binary, when you can just set a size
on the bss section and have the kernel create the appropriate sized
and zero'd mapping?

> That is not the case if the
> initializer is left out and these variables end up in the .bss section.

From my above link, gcc will put globals without initializers into "common."

> From https://en.wikipedia.org/wiki/.bss:
>
> "In computer programming, the name .bss or bss is used by many compilers
> and linkers for the portion of an object file or executable containing
> statically-allocated variables that are not explicitly initialized to any
> value. It is often referred to as the "bss section" or "bss segment".
>
> Typically only the length of the bss section, but no data, is stored in
> the object file."
>
> This is why checkpatch warns if a global or static variable is initialized
> explicitly to zero. From scripts/checkpatch.pl:
>
> our $zero_initializer = qr{(?:(?:0[xX])?0+$Int_type?|NULL|false)\b};
>
> # check for global initialisers.
>
>                 if ($line =~ /^\+$Type\s*$Ident(?:\s+$Modifier)*\s*=\s*($zero_initializer)\s*;/) {
>                         if (ERROR("GLOBAL_INITIALISERS",
>                                   "do not initialise globals to $1\n" . $herecurr) && $fix) {
>                                 $fixed[$fixlinenr] =~ s/(^.$Type\s*$Ident(?:\s+$Modifier)*)\s*=\s*$zero_initializer\s*;/$1;/;
>                         }
>                 }
>
> # check for static initialisers.
>
>                 if ($line =~ /^\+.*\bstatic\s.*=\s*($zero_initializer)\s*;/) {
>                         if (ERROR("INITIALISED_STATIC",
>                                   "do not initialise statics to $1\n" . $herecurr) && $fix) {
>                                 $fixed[$fixlinenr] =~ s/(\bstatic\s.*?)\s*=\s*$zero_initializer\s*;/$1;/;
>                         }
>                 }
>
> Bart.



-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] libosd: Remove ignored __weak attribute

[Bart Van Assche]

On Tue, 2018-10-02 at 15:33 -0700, Nick Desaulniers wrote:
> On Tue, Oct 2, 2018 at 10:57 AM Bart Van Assche <bvanassche@acm.org> wrote:
> > Explicitly initialized global and static variables end up in the .data
> > section and need space in that section.
> 
> Unless the initial value is zero.
> https://godbolt.org/z/curRoO
> 
> So you don't wind up with an increase in binary size simply by having
> global variables initialized to zero, right?  Instead the kernel knows
> to create a zero'd out mapping for bss.  You don't need a run of zeros
> in the binary.
> 
> So I disagree when you said earlier "zero initializers should be left
> out to minimize the size of object files." I assert they don't affect
> the size of the binary.
> 
> If you had many global variables all initialized to zero, why would
> you encode that many zeros in a binary, when you can just set a size
> on the bss section and have the kernel create the appropriate sized
> and zero'd mapping?
> 
> > That is not the case if the
> > initializer is left out and these variables end up in the .bss section.
> 
> From my above link, gcc will put globals without initializers into "common."

No matter what particular compiler versions do with explicit initialization
to zero, the preferred kernel coding style is to leave out such explicit
initialization.

Bart.

Re: [PATCH] libosd: Remove ignored __weak attribute

[Nathan Chancellor]

On Tue, Oct 02, 2018 at 04:06:31PM -0700, Bart Van Assche wrote:
> On Tue, 2018-10-02 at 15:33 -0700, Nick Desaulniers wrote:
> > On Tue, Oct 2, 2018 at 10:57 AM Bart Van Assche <bvanassche@acm.org> wrote:
> > > Explicitly initialized global and static variables end up in the .data
> > > section and need space in that section.
> > 
> > Unless the initial value is zero.
> > https://godbolt.org/z/curRoO
> > 
> > So you don't wind up with an increase in binary size simply by having
> > global variables initialized to zero, right?  Instead the kernel knows
> > to create a zero'd out mapping for bss.  You don't need a run of zeros
> > in the binary.
> > 
> > So I disagree when you said earlier "zero initializers should be left
> > out to minimize the size of object files." I assert they don't affect
> > the size of the binary.
> > 
> > If you had many global variables all initialized to zero, why would
> > you encode that many zeros in a binary, when you can just set a size
> > on the bss section and have the kernel create the appropriate sized
> > and zero'd mapping?
> > 
> > > That is not the case if the
> > > initializer is left out and these variables end up in the .bss section.
> > 
> > From my above link, gcc will put globals without initializers into "common."
> 
> No matter what particular compiler versions do with explicit initialization
> to zero, the preferred kernel coding style is to leave out such explicit
> initialization.
> 
> Bart.

Hi Bart,

I'm sorry if I didn't follow the conclusion of this conversation properly
but this is the below diff you were initially looking for, correct?

If so, Boaz and Nick, do you have any objections if this is v2? I'd like
to get this patch accepted so the warning can be fixed for everyone.

Thanks,
Nathan

================================================================================

diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c
index e19fa883376f..4250f739beb3 100644
--- a/drivers/scsi/osd/osd_initiator.c
+++ b/drivers/scsi/osd/osd_initiator.c
@@ -58,6 +58,8 @@

 enum { OSD_REQ_RETRIES = 1 };

+static const struct osd_obj_id osd_root_object;
+
 MODULE_AUTHOR("Boaz Harrosh <ooo@electrozaur.com>");
 MODULE_DESCRIPTION("open-osd initiator library libosd.ko");
 MODULE_LICENSE("GPL");
diff --git a/drivers/scsi/osd/osd_uld.c b/drivers/scsi/osd/osd_uld.c
index eaf36ccf58db..770c758baaa9 100644
--- a/drivers/scsi/osd/osd_uld.c
+++ b/drivers/scsi/osd/osd_uld.c
@@ -73,6 +73,7 @@

 static const char osd_name[] = "osd";
 static const char *osd_version_string = "open-osd 0.2.1";
+static const struct osd_obj_id osd_root_object;

 MODULE_AUTHOR("Boaz Harrosh <ooo@electrozaur.com>");
 MODULE_DESCRIPTION("open-osd Upper-Layer-Driver osd.ko");
diff --git a/include/scsi/osd_types.h b/include/scsi/osd_types.h
index 48e8a165e136..eb31357ec8b3 100644
--- a/include/scsi/osd_types.h
+++ b/include/scsi/osd_types.h
@@ -28,8 +28,6 @@ struct osd_obj_id {
        osd_id id;
 };

-static const struct __weak osd_obj_id osd_root_object = {0, 0};
-
 struct osd_attr {
        u32 attr_page;
        u32 attr_id;

Re: [PATCH] libosd: Remove ignored __weak attribute

[Nick Desaulniers]

On Thu, Oct 25, 2018 at 2:31 PM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> On Tue, Oct 02, 2018 at 04:06:31PM -0700, Bart Van Assche wrote:
> > On Tue, 2018-10-02 at 15:33 -0700, Nick Desaulniers wrote:
> > > On Tue, Oct 2, 2018 at 10:57 AM Bart Van Assche <bvanassche@acm.org> wrote:
> > > > Explicitly initialized global and static variables end up in the .data
> > > > section and need space in that section.
> > >
> > > Unless the initial value is zero.
> > > https://godbolt.org/z/curRoO
> > >
> > > So you don't wind up with an increase in binary size simply by having
> > > global variables initialized to zero, right?  Instead the kernel knows
> > > to create a zero'd out mapping for bss.  You don't need a run of zeros
> > > in the binary.
> > >
> > > So I disagree when you said earlier "zero initializers should be left
> > > out to minimize the size of object files." I assert they don't affect
> > > the size of the binary.
> > >
> > > If you had many global variables all initialized to zero, why would
> > > you encode that many zeros in a binary, when you can just set a size
> > > on the bss section and have the kernel create the appropriate sized
> > > and zero'd mapping?
> > >
> > > > That is not the case if the
> > > > initializer is left out and these variables end up in the .bss section.
> > >
> > > From my above link, gcc will put globals without initializers into "common."
> >
> > No matter what particular compiler versions do with explicit initialization
> > to zero, the preferred kernel coding style is to leave out such explicit
> > initialization.
> >
> > Bart.
>
> Hi Bart,
>
> I'm sorry if I didn't follow the conclusion of this conversation properly
> but this is the below diff you were initially looking for, correct?
>
> If so, Boaz and Nick, do you have any objections if this is v2? I'd like
> to get this patch accepted so the warning can be fixed for everyone.

Hi Nathan,
Thanks for following up on this.  Bart's note about the one definition
rule is important.  If you define the variable static in two different
translation units, you've suddenly created two different copies
accessible only to their respective translation units.  So it should
be declared extern in one source file (but not defined/initialized),
and defined (non-static) in another.  See below for example.

>
> Thanks,
> Nathan
>
> ================================================================================
>
> diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c
> index e19fa883376f..4250f739beb3 100644
> --- a/drivers/scsi/osd/osd_initiator.c
> +++ b/drivers/scsi/osd/osd_initiator.c
> @@ -58,6 +58,8 @@
>
>  enum { OSD_REQ_RETRIES = 1 };
>
> +static const struct osd_obj_id osd_root_object;

extern const struct osd_obj_id osd_root_object;

> +
>  MODULE_AUTHOR("Boaz Harrosh <ooo@electrozaur.com>");
>  MODULE_DESCRIPTION("open-osd initiator library libosd.ko");
>  MODULE_LICENSE("GPL");
> diff --git a/drivers/scsi/osd/osd_uld.c b/drivers/scsi/osd/osd_uld.c
> index eaf36ccf58db..770c758baaa9 100644
> --- a/drivers/scsi/osd/osd_uld.c
> +++ b/drivers/scsi/osd/osd_uld.c
> @@ -73,6 +73,7 @@
>
>  static const char osd_name[] = "osd";
>  static const char *osd_version_string = "open-osd 0.2.1";
> +static const struct osd_obj_id osd_root_object;

const struct osd_obj_id osd_root_object;

>
>  MODULE_AUTHOR("Boaz Harrosh <ooo@electrozaur.com>");
>  MODULE_DESCRIPTION("open-osd Upper-Layer-Driver osd.ko");
> diff --git a/include/scsi/osd_types.h b/include/scsi/osd_types.h
> index 48e8a165e136..eb31357ec8b3 100644
> --- a/include/scsi/osd_types.h
> +++ b/include/scsi/osd_types.h
> @@ -28,8 +28,6 @@ struct osd_obj_id {
>         osd_id id;
>  };
>
> -static const struct __weak osd_obj_id osd_root_object = {0, 0};
> -

LGTM

>  struct osd_attr {
>         u32 attr_page;
>         u32 attr_id;

That way the linker knows there's only one instance of this struct in
memory, and that the two different translation units are referring to
the same instance.  The other maintainers may have a preference which
translation you define osd_root_object in (I arbitrarily chose
drivers/scsi/osd/osd_uld.c), but if they don't have additional
feedback after some amount of time, I'd assume they're ok with the
above suggestion.  What do you think?

-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] libosd: Remove ignored __weak attribute

[Nathan Chancellor]

On Thu, Oct 25, 2018 at 03:02:13PM -0700, Nick Desaulniers wrote:
> On Thu, Oct 25, 2018 at 2:31 PM Nathan Chancellor
> <natechancellor@gmail.com> wrote:
> >
> > On Tue, Oct 02, 2018 at 04:06:31PM -0700, Bart Van Assche wrote:
> > > On Tue, 2018-10-02 at 15:33 -0700, Nick Desaulniers wrote:
> > > > On Tue, Oct 2, 2018 at 10:57 AM Bart Van Assche <bvanassche@acm.org> wrote:
> > > > > Explicitly initialized global and static variables end up in the .data
> > > > > section and need space in that section.
> > > >
> > > > Unless the initial value is zero.
> > > > https://godbolt.org/z/curRoO
> > > >
> > > > So you don't wind up with an increase in binary size simply by having
> > > > global variables initialized to zero, right?  Instead the kernel knows
> > > > to create a zero'd out mapping for bss.  You don't need a run of zeros
> > > > in the binary.
> > > >
> > > > So I disagree when you said earlier "zero initializers should be left
> > > > out to minimize the size of object files." I assert they don't affect
> > > > the size of the binary.
> > > >
> > > > If you had many global variables all initialized to zero, why would
> > > > you encode that many zeros in a binary, when you can just set a size
> > > > on the bss section and have the kernel create the appropriate sized
> > > > and zero'd mapping?
> > > >
> > > > > That is not the case if the
> > > > > initializer is left out and these variables end up in the .bss section.
> > > >
> > > > From my above link, gcc will put globals without initializers into "common."
> > >
> > > No matter what particular compiler versions do with explicit initialization
> > > to zero, the preferred kernel coding style is to leave out such explicit
> > > initialization.
> > >
> > > Bart.
> >
> > Hi Bart,
> >
> > I'm sorry if I didn't follow the conclusion of this conversation properly
> > but this is the below diff you were initially looking for, correct?
> >
> > If so, Boaz and Nick, do you have any objections if this is v2? I'd like
> > to get this patch accepted so the warning can be fixed for everyone.
> 
> Hi Nathan,
> Thanks for following up on this.  Bart's note about the one definition
> rule is important.  If you define the variable static in two different
> translation units, you've suddenly created two different copies
> accessible only to their respective translation units.  So it should
> be declared extern in one source file (but not defined/initialized),
> and defined (non-static) in another.  See below for example.
> 

Hi Nick,

I just want to make sure I understand what is going on here.

Doesn't the first part already happen because osd_root_object is
declared static in osd_types.h? I tried this little simple example of
adding a 'static const' variable to a header file and using it in two
separate files/functions. When compiled together, they point to two
different locations in memory.

==============================================

$ clang -std=gnu89 main.c test1.c test2.c
$ ./a.out
test in test1(): 0x55b4df3a001c
test in test2(): 0x55b4df3a003c

==============================================

main.c:

#include "test.h"

int main(void) {
        test1();
        test2();
}

==============================================

test1.c:

#include <stdio.h>
#include "test.h"

void test1() {
    printf("test in test1(): %p\n", &test);
}

==============================================

test2.c:

#include <stdio.h>
#include "test.h"

void test2() {
    printf("test in test2(): %p\n", &test);
}

==============================================

test.h:

struct test_struct {
    int a;
    int b;
};

static const struct test_struct test = {0, 0};
void test1();
void test2();

==============================================

If that is the case, could your suggested change result in a functional
change given that the code would now refer to the same osd_root_object?
This isn't necessarily a problem, especially since it sounds like not
referring to the same object could be a bug, but I want to make sure
that's what is intended by these changes, which I'll be happy to spin up
in a v2.

If I am thinking about this incorrectly or my example is wrong in any
way, please let me know. I'm trying to soak up all of this knowledge
so I can be a better contributor.

Thanks for the reply and explanation!
Nathan

> >
> > Thanks,
> > Nathan
> >
> > ================================================================================
> >
> > diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c
> > index e19fa883376f..4250f739beb3 100644
> > --- a/drivers/scsi/osd/osd_initiator.c
> > +++ b/drivers/scsi/osd/osd_initiator.c
> > @@ -58,6 +58,8 @@
> >
> >  enum { OSD_REQ_RETRIES = 1 };
> >
> > +static const struct osd_obj_id osd_root_object;
> 
> extern const struct osd_obj_id osd_root_object;
> 
> > +
> >  MODULE_AUTHOR("Boaz Harrosh <ooo@electrozaur.com>");
> >  MODULE_DESCRIPTION("open-osd initiator library libosd.ko");
> >  MODULE_LICENSE("GPL");
> > diff --git a/drivers/scsi/osd/osd_uld.c b/drivers/scsi/osd/osd_uld.c
> > index eaf36ccf58db..770c758baaa9 100644
> > --- a/drivers/scsi/osd/osd_uld.c
> > +++ b/drivers/scsi/osd/osd_uld.c
> > @@ -73,6 +73,7 @@
> >
> >  static const char osd_name[] = "osd";
> >  static const char *osd_version_string = "open-osd 0.2.1";
> > +static const struct osd_obj_id osd_root_object;
> 
> const struct osd_obj_id osd_root_object;
> 
> >
> >  MODULE_AUTHOR("Boaz Harrosh <ooo@electrozaur.com>");
> >  MODULE_DESCRIPTION("open-osd Upper-Layer-Driver osd.ko");
> > diff --git a/include/scsi/osd_types.h b/include/scsi/osd_types.h
> > index 48e8a165e136..eb31357ec8b3 100644
> > --- a/include/scsi/osd_types.h
> > +++ b/include/scsi/osd_types.h
> > @@ -28,8 +28,6 @@ struct osd_obj_id {
> >         osd_id id;
> >  };
> >
> > -static const struct __weak osd_obj_id osd_root_object = {0, 0};
> > -
> 
> LGTM
> 
> >  struct osd_attr {
> >         u32 attr_page;
> >         u32 attr_id;
> 
> That way the linker knows there's only one instance of this struct in
> memory, and that the two different translation units are referring to
> the same instance.  The other maintainers may have a preference which
> translation you define osd_root_object in (I arbitrarily chose
> drivers/scsi/osd/osd_uld.c), but if they don't have additional
> feedback after some amount of time, I'd assume they're ok with the
> above suggestion.  What do you think?
> 
> -- 
> Thanks,
> ~Nick Desaulniers

Re: [PATCH] libosd: Remove ignored __weak attribute

[Nick Desaulniers]

On Thu, Oct 25, 2018 at 3:55 PM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> On Thu, Oct 25, 2018 at 03:02:13PM -0700, Nick Desaulniers wrote:
> > On Thu, Oct 25, 2018 at 2:31 PM Nathan Chancellor
> > <natechancellor@gmail.com> wrote:
> > >
> > > On Tue, Oct 02, 2018 at 04:06:31PM -0700, Bart Van Assche wrote:
> > > > On Tue, 2018-10-02 at 15:33 -0700, Nick Desaulniers wrote:
> > > > > On Tue, Oct 2, 2018 at 10:57 AM Bart Van Assche <bvanassche@acm.org> wrote:
> > > > > > Explicitly initialized global and static variables end up in the .data
> > > > > > section and need space in that section.
> > > > >
> > > > > Unless the initial value is zero.
> > > > > https://godbolt.org/z/curRoO
> > > > >
> > > > > So you don't wind up with an increase in binary size simply by having
> > > > > global variables initialized to zero, right?  Instead the kernel knows
> > > > > to create a zero'd out mapping for bss.  You don't need a run of zeros
> > > > > in the binary.
> > > > >
> > > > > So I disagree when you said earlier "zero initializers should be left
> > > > > out to minimize the size of object files." I assert they don't affect
> > > > > the size of the binary.
> > > > >
> > > > > If you had many global variables all initialized to zero, why would
> > > > > you encode that many zeros in a binary, when you can just set a size
> > > > > on the bss section and have the kernel create the appropriate sized
> > > > > and zero'd mapping?
> > > > >
> > > > > > That is not the case if the
> > > > > > initializer is left out and these variables end up in the .bss section.
> > > > >
> > > > > From my above link, gcc will put globals without initializers into "common."
> > > >
> > > > No matter what particular compiler versions do with explicit initialization
> > > > to zero, the preferred kernel coding style is to leave out such explicit
> > > > initialization.
> > > >
> > > > Bart.
> > >
> > > Hi Bart,
> > >
> > > I'm sorry if I didn't follow the conclusion of this conversation properly
> > > but this is the below diff you were initially looking for, correct?
> > >
> > > If so, Boaz and Nick, do you have any objections if this is v2? I'd like
> > > to get this patch accepted so the warning can be fixed for everyone.
> >
> > Hi Nathan,
> > Thanks for following up on this.  Bart's note about the one definition
> > rule is important.  If you define the variable static in two different
> > translation units, you've suddenly created two different copies
> > accessible only to their respective translation units.  So it should
> > be declared extern in one source file (but not defined/initialized),
> > and defined (non-static) in another.  See below for example.
> >
>
> Hi Nick,
>
> I just want to make sure I understand what is going on here.
>
> Doesn't the first part already happen because osd_root_object is
> declared static in osd_types.h? I tried this little simple example of
> adding a 'static const' variable to a header file and using it in two
> separate files/functions. When compiled together, they point to two
> different locations in memory.
>
> ==============================================
>
> $ clang -std=gnu89 main.c test1.c test2.c
> $ ./a.out
> test in test1(): 0x55b4df3a001c
> test in test2(): 0x55b4df3a003c
>
> ==============================================
>
> main.c:
>
> #include "test.h"
>
> int main(void) {
>         test1();
>         test2();
> }
>
> ==============================================
>
> test1.c:
>
> #include <stdio.h>
> #include "test.h"
>
> void test1() {
>     printf("test in test1(): %p\n", &test);
> }
>
> ==============================================
>
> test2.c:
>
> #include <stdio.h>
> #include "test.h"
>
> void test2() {
>     printf("test in test2(): %p\n", &test);
> }
>
> ==============================================
>
> test.h:
>
> struct test_struct {
>     int a;
>     int b;
> };
>
> static const struct test_struct test = {0, 0};
> void test1();
> void test2();
>
> ==============================================
>
> If that is the case, could your suggested change result in a functional
> change given that the code would now refer to the same osd_root_object?

It's hard to say without knowing the original intent of the code.
From the variable's identifier and fact that it's global, I *assume*
that we want only 1 struct osd_obj_id which is the root, hence the
identifier `osd_root_object`.  It has 4 references in the entire
kernel; it doesn't make sense to my why those references would want to
be referring to two different instances of `osd_root_object`.  Maybe
the maintainers can clarify if 2 instances is the intent?

Further complicated is the use of the __weak attribute AND the
compiler flag -fno-common (which the kernel sets in the top level
Makefile).  Also, it seems that ODR is a C++ concept; it's not clear
to me if there's semantic differences with C or not (I don't think so
in this case, but I've learned not to bet on subtle semantic
differences between the languages not existing).

__attribute__((weak)) AND static on a global variable declared in a
header raises many red flags to me.  Was weak added to work around an
ODR link error?

If creating one instance of this variable is a functional change, I
can't help but suspect the original code was wrong.  But maybe Bart,
Boaz, or Christoph can clarify or have more thoughts on this?  Looks
like Boaz added this header in commit de258bf5e638 ("[SCSI] libosd:
OSDv1 Headers").

> This isn't necessarily a problem, especially since it sounds like not
> referring to the same object could be a bug, but I want to make sure
> that's what is intended by these changes, which I'll be happy to spin up
> in a v2.
>
> If I am thinking about this incorrectly or my example is wrong in any
> way, please let me know. I'm trying to soak up all of this knowledge
> so I can be a better contributor.
>
> Thanks for the reply and explanation!
> Nathan
>
> > >
> > > Thanks,
> > > Nathan
> > >
> > > ================================================================================
> > >
> > > diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c
> > > index e19fa883376f..4250f739beb3 100644
> > > --- a/drivers/scsi/osd/osd_initiator.c
> > > +++ b/drivers/scsi/osd/osd_initiator.c
> > > @@ -58,6 +58,8 @@
> > >
> > >  enum { OSD_REQ_RETRIES = 1 };
> > >
> > > +static const struct osd_obj_id osd_root_object;
> >
> > extern const struct osd_obj_id osd_root_object;
> >
> > > +
> > >  MODULE_AUTHOR("Boaz Harrosh <ooo@electrozaur.com>");
> > >  MODULE_DESCRIPTION("open-osd initiator library libosd.ko");
> > >  MODULE_LICENSE("GPL");
> > > diff --git a/drivers/scsi/osd/osd_uld.c b/drivers/scsi/osd/osd_uld.c
> > > index eaf36ccf58db..770c758baaa9 100644
> > > --- a/drivers/scsi/osd/osd_uld.c
> > > +++ b/drivers/scsi/osd/osd_uld.c
> > > @@ -73,6 +73,7 @@
> > >
> > >  static const char osd_name[] = "osd";
> > >  static const char *osd_version_string = "open-osd 0.2.1";
> > > +static const struct osd_obj_id osd_root_object;
> >
> > const struct osd_obj_id osd_root_object;
> >
> > >
> > >  MODULE_AUTHOR("Boaz Harrosh <ooo@electrozaur.com>");
> > >  MODULE_DESCRIPTION("open-osd Upper-Layer-Driver osd.ko");
> > > diff --git a/include/scsi/osd_types.h b/include/scsi/osd_types.h
> > > index 48e8a165e136..eb31357ec8b3 100644
> > > --- a/include/scsi/osd_types.h
> > > +++ b/include/scsi/osd_types.h
> > > @@ -28,8 +28,6 @@ struct osd_obj_id {
> > >         osd_id id;
> > >  };
> > >
> > > -static const struct __weak osd_obj_id osd_root_object = {0, 0};
> > > -
> >
> > LGTM
> >
> > >  struct osd_attr {
> > >         u32 attr_page;
> > >         u32 attr_id;
> >
> > That way the linker knows there's only one instance of this struct in
> > memory, and that the two different translation units are referring to
> > the same instance.  The other maintainers may have a preference which
> > translation you define osd_root_object in (I arbitrarily chose
> > drivers/scsi/osd/osd_uld.c), but if they don't have additional
> > feedback after some amount of time, I'd assume they're ok with the
> > above suggestion.  What do you think?
> >
> > --
> > Thanks,
> > ~Nick Desaulniers



-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] libosd: Remove ignored __weak attribute

[Bart Van Assche]

On Fri, 2018-10-26 at 10:54 -0700, Nick Desaulniers wrote:
> If creating one instance of this variable is a functional change, I
> can't help but suspect the original code was wrong.  But maybe Bart,
> Boaz, or Christoph can clarify or have more thoughts on this?  Looks
> like Boaz added this header in commit de258bf5e638 ("[SCSI] libosd:
> OSDv1 Headers").

Hi Nick and Nathan,

Had you noticed the following e-mail from early October:
https://marc.info/?l=linux-kernel&m=153849955503249?

Thanks,

Bart.

Re: [PATCH] libosd: Remove ignored __weak attribute

[Nathan Chancellor]

On Fri, Oct 26, 2018 at 11:01:48AM -0700, Bart Van Assche wrote:
> On Fri, 2018-10-26 at 10:54 -0700, Nick Desaulniers wrote:
> > If creating one instance of this variable is a functional change, I
> > can't help but suspect the original code was wrong.  But maybe Bart,
> > Boaz, or Christoph can clarify or have more thoughts on this?  Looks
> > like Boaz added this header in commit de258bf5e638 ("[SCSI] libosd:
> > OSDv1 Headers").
> 
> Hi Nick and Nathan,
> 
> Had you noticed the following e-mail from early October:
> https://marc.info/?l=linux-kernel&m=153849955503249?
> 
> Thanks,
> 
> Bart.

Hi Bart,

I had but there doesn't seem to be any reply to it so I wasn't sure if
there was a final consensus on removing the driver. If that's the route
that everyone wants to go, then I guess we don't need to continue to
debate this patch.

Thanks for the heads up,
Nathan

Re: [PATCH] libosd: Remove ignored __weak attribute

[Nick Desaulniers]

On Fri, Oct 26, 2018 at 11:05 AM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> On Fri, Oct 26, 2018 at 11:01:48AM -0700, Bart Van Assche wrote:
> > On Fri, 2018-10-26 at 10:54 -0700, Nick Desaulniers wrote:
> > > If creating one instance of this variable is a functional change, I
> > > can't help but suspect the original code was wrong.  But maybe Bart,
> > > Boaz, or Christoph can clarify or have more thoughts on this?  Looks
> > > like Boaz added this header in commit de258bf5e638 ("[SCSI] libosd:
> > > OSDv1 Headers").
> >
> > Hi Nick and Nathan,
> >
> > Had you noticed the following e-mail from early October:
> > https://marc.info/?l=linux-kernel&m=153849955503249?
> >
> > Thanks,
> >
> > Bart.
>
> Hi Bart,
>
> I had but there doesn't seem to be any reply to it so I wasn't sure if
> there was a final consensus on removing the driver. If that's the route
> that everyone wants to go, then I guess we don't need to continue to
> debate this patch.
>
> Thanks for the heads up,
> Nathan

I've staged a RFC patch here:
https://github.com/ClangBuiltLinux/linux/commit/03817d982606e2db143d23a8a55e97de6de6e0c2

+ Linus
I'm not about the process of removing code from the kernel.  Doesn't
that violate the "thou shalt not break userspace" rule?  But code does
get deleted from the kernel... ex:
http://lkml.iu.edu/hypermail/linux/kernel/1804.1/06654.html

-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] libosd: Remove ignored __weak attribute

[Linus Torvalds]

On Fri, Oct 26, 2018 at 11:32 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> + Linus
> I'm not about the process of removing code from the kernel.  Doesn't
> that violate the "thou shalt not break userspace" rule?

The only thing that breaks the "thou shalt not break userspace" rule
is fairly simple: things that break user space.

Does removing the code break for somebody? If so we don't do it. But
if nobody notices because nobody uses, it's fine.

Basically, there is no "theoretical" rule about what breaks user space
or not. In particular, the rule is *not* that you can't change ABI.
You can do any change you want that changes a kernel exported ABI,
just as long as nobody actually notices the change.

But in practice, it's often _much_ more work to try to figure out
whether something breaks somebody than it is to just say "don't change
behavior", so 99% of the time, the rule ends up being just "try to
avoid intentionally changing behavior, because you'll likely get it
wrong".

              Linus

Re: [PATCH] libosd: Remove ignored __weak attribute

[Nick Desaulniers]

On Fri, Oct 26, 2018 at 12:22 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Fri, Oct 26, 2018 at 11:32 AM Nick Desaulniers
> <ndesaulniers@google.com> wrote:
> >
> > + Linus
> > I'm not about the process of removing code from the kernel.  Doesn't
> > that violate the "thou shalt not break userspace" rule?
>
> The only thing that breaks the "thou shalt not break userspace" rule
> is fairly simple: things that break user space.

Is removing a filesystem considered a userspace breakage?  If you have
an exofs mount, and you upgrade to a kernel with the RFC patch, you
will no longer be able to mount that type of FS.  Userspace code that
expects to mount an exofs FS would now always fail.  Maybe this is
equivalent to removing a previously supported mount option.
/proc/filesystems might have one less option.

So I guess this is different than dropping support for an architecture
since you cannot produce an updated kernel image, as opposed to this
RFC which would make existing drives fail to mount?

>
> Does removing the code break for somebody? If so we don't do it. But
> if nobody notices because nobody uses, it's fine.

Right, but it seems like a catch 22; it's not possible to prove that
nobody will notice because they are not using it.

Like the expression:
If a tree falls in a forest and no one is around to hear it, does it
make a sound?

If no one is using a kernel feature and its removed, will anyone notice?

If the maintainer would like to remove support for their subsystem,
should it continue to remain in the tree?  Code is never truly
deleted, and is relatively painless to resurrect with git.

Solutions that come to mind immediately:
* Just land the small fix discussed earlier in the thread. These
subsystems continue to exist.  Maybe this question comes up again for
this subsystem in a few years.  Christoph alludes to my question not
being the first time this was asked of this subsystem.
* Put out some kind of intent to deprecate, hoping to get feedback on
possible users that would be affected. Land the deletion patches in
-next.  This approach always runs the risk of not posting in the right
venue; and users can't be bothered to check for intent to deprecate
notices.
* Just delete it. Resurrect if users report use of this FS.

What are your thoughts?

>
> Basically, there is no "theoretical" rule about what breaks user space
> or not. In particular, the rule is *not* that you can't change ABI.
> You can do any change you want that changes a kernel exported ABI,
> just as long as nobody actually notices the change.
>
> But in practice, it's often _much_ more work to try to figure out
> whether something breaks somebody than it is to just say "don't change
> behavior", so 99% of the time, the rule ends up being just "try to
> avoid intentionally changing behavior, because you'll likely get it
> wrong".

I agree semantic changes to an existing API are problematic (not just
within the kernel), but do you consider deletion or removal of
parameters in the same category?

Thank you for your insight on this.
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] libosd: Remove ignored __weak attribute

[Linus Torvalds]

On Fri, Oct 26, 2018 at 1:06 PM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> Is removing a filesystem considered a userspace breakage?

Yes - if a user notices.

The key word is *USER*.

Note that it's not "user space". It's not about _programs_ noticing,
it's literally about users and their workflows.

If some change breaks a real user workflow, it needs to be reverted.

So this is not about ABI or anything like that. We've had cases where
the ABI stayed the same, but the order of device probing changed, and
that broke peoples setups (because now /dev/sdb and /dev/sda switched
places), and we had to revert.

It's literally about "if a user upgrades a kernel, and something no
longer works, it's a regression".

In general, a good idea is "if you have to wonder about it, just don't
do it".  Because it turns out that users are odd, and often do odd
things much after you'd have thought they'd have long since switched
to more modern hardware or filesystems.

                      Linus

Re: [PATCH] libosd: Remove ignored __weak attribute

[Nick Desaulniers]

On Fri, Oct 26, 2018 at 11:01 AM Bart Van Assche <bvanassche@acm.org> wrote:
>
> On Fri, 2018-10-26 at 10:54 -0700, Nick Desaulniers wrote:
> > If creating one instance of this variable is a functional change, I
> > can't help but suspect the original code was wrong.  But maybe Bart,
> > Boaz, or Christoph can clarify or have more thoughts on this?  Looks
> > like Boaz added this header in commit de258bf5e638 ("[SCSI] libosd:
> > OSDv1 Headers").
>
> Hi Nick and Nathan,
>
> Had you noticed the following e-mail from early October:
> https://marc.info/?l=linux-kernel&m=153849955503249?

From this subthread with Linus, removal of the exofs fs and scsi osd
code would be a user visible change and is not an option. See:
https://lkml.org/lkml/2018/10/27/3
https://lkml.org/lkml/2018/10/27/44

So we should pursue this much smaller fixup.  Boaz, can you clarify
https://lkml.org/lkml/2018/10/26/682
specifically:

> If creating one instance of this variable is a functional change, I
> can't help but suspect the original code was wrong.  But maybe Bart,
> Boaz, or Christoph can clarify or have more thoughts on this?  Looks
> like Boaz added this header in commit de258bf5e638 ("[SCSI] libosd:
> OSDv1 Headers").

-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] libosd: Remove ignored __weak attribute

[Nick Desaulniers]

On Fri, Oct 26, 2018 at 1:42 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Fri, Oct 26, 2018 at 1:06 PM Nick Desaulniers
> <ndesaulniers@google.com> wrote:
> >
> > Is removing a filesystem considered a userspace breakage?
>
> Yes - if a user notices.
>
> The key word is *USER*.
>
> Note that it's not "user space". It's not about _programs_ noticing,
> it's literally about users and their workflows.
>
> If some change breaks a real user workflow, it needs to be reverted.
>
> So this is not about ABI or anything like that. We've had cases where
> the ABI stayed the same, but the order of device probing changed, and
> that broke peoples setups (because now /dev/sdb and /dev/sda switched
> places), and we had to revert.
>
> It's literally about "if a user upgrades a kernel, and something no
> longer works, it's a regression".
>
> In general, a good idea is "if you have to wonder about it, just don't
> do it".  Because it turns out that users are odd, and often do odd
> things much after you'd have thought they'd have long since switched
> to more modern hardware or filesystems.
>
>                       Linus

Makes sense and is a consistent stance.  Thanks for clarifying.  Will
pursue the smaller fix in the other subthread.
https://lkml.org/lkml/2018/10/27/55

-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] libosd: Remove ignored __weak attribute

[Bart Van Assche]

On Fri, 2018-10-26 at 14:00 -0700, Nick Desaulniers wrote:
> On Fri, Oct 26, 2018 at 11:01 AM Bart Van Assche <bvanassche@acm.org> wrote:
> > 
> > On Fri, 2018-10-26 at 10:54 -0700, Nick Desaulniers wrote:
> > > If creating one instance of this variable is a functional change, I
> > > can't help but suspect the original code was wrong.  But maybe Bart,
> > > Boaz, or Christoph can clarify or have more thoughts on this?  Looks
> > > like Boaz added this header in commit de258bf5e638 ("[SCSI] libosd:
> > > OSDv1 Headers").
> > 
> > Hi Nick and Nathan,
> > 
> > Had you noticed the following e-mail from early October:
> > https://marc.info/?l=linux-kernel&m=153849955503249?
> 
> From this subthread with Linus, removal of the exofs fs and scsi osd
> code would be a user visible change and is not an option. See:
> https://lkml.org/lkml/2018/10/27/3
> https://lkml.org/lkml/2018/10/27/44

Hi Nick,

Linus wrote that removing a filesystem is considered a userspace breakage
if a user notices. The key part is "if a user notices". Who are the exofs
users?

Bart.

Re: [PATCH] libosd: Remove ignored __weak attribute

[Nick Desaulniers]

On Fri, Oct 26, 2018 at 2:30 PM Bart Van Assche <bvanassche@acm.org> wrote:
>
> On Fri, 2018-10-26 at 14:00 -0700, Nick Desaulniers wrote:
> > On Fri, Oct 26, 2018 at 11:01 AM Bart Van Assche <bvanassche@acm.org> wrote:
> > >
> > > On Fri, 2018-10-26 at 10:54 -0700, Nick Desaulniers wrote:
> > > > If creating one instance of this variable is a functional change, I
> > > > can't help but suspect the original code was wrong.  But maybe Bart,
> > > > Boaz, or Christoph can clarify or have more thoughts on this?  Looks
> > > > like Boaz added this header in commit de258bf5e638 ("[SCSI] libosd:
> > > > OSDv1 Headers").
> > >
> > > Hi Nick and Nathan,
> > >
> > > Had you noticed the following e-mail from early October:
> > > https://marc.info/?l=linux-kernel&m=153849955503249?
> >
> > From this subthread with Linus, removal of the exofs fs and scsi osd
> > code would be a user visible change and is not an option. See:
> > https://lkml.org/lkml/2018/10/27/3
> > https://lkml.org/lkml/2018/10/27/44
>
> Hi Nick,
>
> Linus wrote that removing a filesystem is considered a userspace breakage
> if a user notices. The key part is "if a user notices". Who are the exofs
> users?

See my thoughts on this in https://lkml.org/lkml/2018/10/27/27.
Particularly the part about the IMO catch 22.

Neither you nor I can claim "there are none."
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] libosd: Remove ignored __weak attribute

[Bart Van Assche]

On Fri, 2018-10-26 at 14:36 -0700, Nick Desaulniers wrote:
> On Fri, Oct 26, 2018 at 2:30 PM Bart Van Assche <bvanassche@acm.org> wrote:
> > 
> > On Fri, 2018-10-26 at 14:00 -0700, Nick Desaulniers wrote:
> > > On Fri, Oct 26, 2018 at 11:01 AM Bart Van Assche <bvanassche@acm.org> wrote:
> > > > 
> > > > On Fri, 2018-10-26 at 10:54 -0700, Nick Desaulniers wrote:
> > > > > If creating one instance of this variable is a functional change, I
> > > > > can't help but suspect the original code was wrong.  But maybe Bart,
> > > > > Boaz, or Christoph can clarify or have more thoughts on this?  Looks
> > > > > like Boaz added this header in commit de258bf5e638 ("[SCSI] libosd:
> > > > > OSDv1 Headers").
> > > > 
> > > > Hi Nick and Nathan,
> > > > 
> > > > Had you noticed the following e-mail from early October:
> > > > https://marc.info/?l=linux-kernel&m=153849955503249?
> > > 
> > > From this subthread with Linus, removal of the exofs fs and scsi osd
> > > code would be a user visible change and is not an option. See:
> > > https://lkml.org/lkml/2018/10/27/3
> > > https://lkml.org/lkml/2018/10/27/44
> > 
> > Hi Nick,
> > 
> > Linus wrote that removing a filesystem is considered a userspace breakage
> > if a user notices. The key part is "if a user notices". Who are the exofs
> > users?
> 
> See my thoughts on this in https://lkml.org/lkml/2018/10/27/27.
> Particularly the part about the IMO catch 22.
> 
> Neither you nor I can claim "there are none."

That's not completely correct. The standard approach to check whether or not
a driver is still being used is to check its git history. If the number of
contributors is low and it was several years ago that a new feature was added
or a bug has been fixed it is likely that nobody is using that driver anymore.

Bart.

Re: [PATCH] libosd: Remove ignored __weak attribute

[Nick Desaulniers]

On Fri, Oct 26, 2018 at 2:59 PM Bart Van Assche <bvanassche@acm.org> wrote:
>
> On Fri, 2018-10-26 at 14:36 -0700, Nick Desaulniers wrote:
> > On Fri, Oct 26, 2018 at 2:30 PM Bart Van Assche <bvanassche@acm.org> wrote:
> > >
> > > On Fri, 2018-10-26 at 14:00 -0700, Nick Desaulniers wrote:
> > > > On Fri, Oct 26, 2018 at 11:01 AM Bart Van Assche <bvanassche@acm.org> wrote:
> > > > >
> > > > > On Fri, 2018-10-26 at 10:54 -0700, Nick Desaulniers wrote:
> > > > > > If creating one instance of this variable is a functional change, I
> > > > > > can't help but suspect the original code was wrong.  But maybe Bart,
> > > > > > Boaz, or Christoph can clarify or have more thoughts on this?  Looks
> > > > > > like Boaz added this header in commit de258bf5e638 ("[SCSI] libosd:
> > > > > > OSDv1 Headers").
> > > > >
> > > > > Hi Nick and Nathan,
> > > > >
> > > > > Had you noticed the following e-mail from early October:
> > > > > https://marc.info/?l=linux-kernel&m=153849955503249?
> > > >
> > > > From this subthread with Linus, removal of the exofs fs and scsi osd
> > > > code would be a user visible change and is not an option. See:
> > > > https://lkml.org/lkml/2018/10/27/3
> > > > https://lkml.org/lkml/2018/10/27/44
> > >
> > > Hi Nick,
> > >
> > > Linus wrote that removing a filesystem is considered a userspace breakage
> > > if a user notices. The key part is "if a user notices". Who are the exofs
> > > users?
> >
> > See my thoughts on this in https://lkml.org/lkml/2018/10/27/27.
> > Particularly the part about the IMO catch 22.
> >
> > Neither you nor I can claim "there are none."
>
> That's not completely correct. The standard approach to check whether or not
> a driver is still being used is to check its git history. If the number of
> contributors is low and it was several years ago that a new feature was added
> or a bug has been fixed it is likely that nobody is using that driver anymore.
>
> Bart.
>

Bart,
I don't disagree with you, I just don't see how what you state can be
reconciled with Linus' response in
https://lkml.org/lkml/2018/10/27/44.  Those two viewpoints seem
incompatible to me, but maybe there's a nuance I'm missing?

Nathan and I are just pointing out a small fix to eliminate a small
warning, deleting all this code does kind of feels like "throwing out
the baby with the bath water." A nuclear option for what would be a
small change otherwise.  Maybe it's good to discuss the EOL for
exofs/osd, but can we please decouple that conversation from the small
change Nathan and I are proposing?
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] libosd: Remove ignored __weak attribute

[Bart Van Assche]

On Fri, 2018-10-26 at 15:07 -0700, Nick Desaulniers wrote:
> I don't disagree with you, I just don't see how what you state can be
> reconciled with Linus' response in
> https://lkml.org/lkml/2018/10/27/44.  Those two viewpoints seem
> incompatible to me, but maybe there's a nuance I'm missing?

I don't think there is any disagreement nor that there are any conflicting
viewpoints. As explained in previous e-mails it is unlikely that anyone is
using these kernel drivers and as far as I know Linus is OK with removing
unused kernel drivers.

> Nathan and I are just pointing out a small fix to eliminate a small
> warning, deleting all this code does kind of feels like "throwing out
> the baby with the bath water." A nuclear option for what would be a
> small change otherwise.  Maybe it's good to discuss the EOL for
> exofs/osd, but can we please decouple that conversation from the small
> change Nathan and I are proposing?

Removing a kernel driver is not a nuclear option. You may not be aware of
this but it happens all the time. From a maintainer point of view it is a
very sensible action because there are people who keep submitting cleanup
patches for kernel drivers they do not use themselves. Every individual
patch needs some attention and hence causes some work for a kernel
maintainer. Removing kernel drivers that are not used helps to reduce the
workload of a maintainer and hence is a rational action. Additionally, if
anyone would ever complain about removal of a kernel driver, it can be
brought back by reverting the commit through which it has been removed.
Martin, please reply if you see this differently.

Bart.

Re: [PATCH] libosd: Remove ignored __weak attribute

[Theodore Y. Ts'o]

On Fri, Oct 26, 2018 at 03:07:39PM -0700, Nick Desaulniers wrote:
> > That's not completely correct. The standard approach to check whether or not
> > a driver is still being used is to check its git history. If the number of
> > contributors is low and it was several years ago that a new feature was added
> > or a bug has been fixed it is likely that nobody is using that driver anymore.
> 
> I don't disagree with you, I just don't see how what you state can be
> reconciled with Linus' response in
> https://lkml.org/lkml/2018/10/27/44.  Those two viewpoints seem
> incompatible to me, but maybe there's a nuance I'm missing?

So a couple of observations.  Obviously, drivers, file systems and
architectures *have* been removed.  It can be done; sometimes if it
can be demonstrate that it can't possibly work (for example, due to
bitrot, the kernel would immediately crashed if anyone tried to use
the code in question :-).

In other cases, drivers has been removed through the staging
subsystem, sometimes by adding a "depends on BROKEN" in the Kconfig
file, and seeing if anyone complains --- since removing a "depends on
BROKEN" line in Kconfig is even easier than doing reverting a git
commit (especially if the user downloaded a tarball instead of doing a
git clone).

If you've done your due diligence then the chances that you have to
revert a change which disables and later removes the dead code can be
pushed close to zero.  The question is whether it's worth the effort.

> Nathan and I are just pointing out a small fix to eliminate a small
> warning, deleting all this code does kind of feels like "throwing out
> the baby with the bath water." A nuclear option for what would be a
> small change otherwise.  Maybe it's good to discuss the EOL for
> exofs/osd, but can we please decouple that conversation from the small
> change Nathan and I are proposing?

The second observation I'll make is that if someone is proposing a
cleanup patch, it's unfair to dump on the person proposing the cleanup
patch the (non-trivial) effort to drop a driver/file system/subsystem.

If the maintainer wants to drop a driver/file system, that should be
the maintainer's responsibiltiy; not someone proposing a
cleanup/maintenance patch.

						- Ted

Re: [PATCH] libosd: Remove ignored __weak attribute

[Bart Van Assche]

On 10/26/18 8:35 PM, Theodore Y. Ts'o wrote:
> The second observation I'll make is that if someone is proposing a
> cleanup patch, it's unfair to dump on the person proposing the cleanup
> patch the (non-trivial) effort to drop a driver/file system/subsystem.

Hi Ted,

Maybe I was not clear enough. It never was my intention to suggest that 
Nick or Nathan should remove the OSD code. This is something I'm willing 
to do myself. BTW, I'm still waiting for someone to explain me why the 
patch at the start of this thread was submitted by people who never have 
used the libosd driver and who do not have any plans to use it ever.

> If the maintainer wants to drop a driver/file system, that should be
> the maintainer's responsibiltiy; not someone proposing a
> cleanup/maintenance patch.

I think anyone who makes tree-wide changes has the freedom to suggest to 
remove a driver. Having to modify drivers that are no longer maintained 
when doing tree-wide changes can be a real pain.

Additionally, you may have missed earlier discussions on the linux-scsi 
mailing list about this driver. The first time it was suggested to 
remove this driver was several years ago. The outcome of a discussion of 
a few weeks ago is that there is agreement about the removal of this 
driver. See also the following messages:
* https://www.spinics.net/lists/linux-scsi/msg123738.html
* https://www.spinics.net/lists/linux-scsi/msg123742.html

Bart.

Re: [PATCH] libosd: Remove ignored __weak attribute

[Nathan Chancellor]

On Fri, Oct 26, 2018 at 11:15:11PM -0700, Bart Van Assche wrote:
> On 10/26/18 8:35 PM, Theodore Y. Ts'o wrote:
> > The second observation I'll make is that if someone is proposing a
> > cleanup patch, it's unfair to dump on the person proposing the cleanup
> > patch the (non-trivial) effort to drop a driver/file system/subsystem.
> 
> Hi Ted,
> 
> Maybe I was not clear enough. It never was my intention to suggest that Nick
> or Nathan should remove the OSD code. This is something I'm willing to do
> myself. BTW, I'm still waiting for someone to explain me why the patch at
> the start of this thread was submitted by people who never have used the
> libosd driver and who do not have any plans to use it ever.
> 

Hi Bart,

We've been cleaning up Clang warnings seen in various configurations. In
this case, I believe this warning shows up in an arm64 allyesconfig
build (would probably show up in an x86_64 one too but I'm not going to
test right now).

More info: https://github.com/ClangBuiltLinux/linux/issues/58

Cheers,
Nathan

> > If the maintainer wants to drop a driver/file system, that should be
> > the maintainer's responsibiltiy; not someone proposing a
> > cleanup/maintenance patch.
> 
> I think anyone who makes tree-wide changes has the freedom to suggest to
> remove a driver. Having to modify drivers that are no longer maintained when
> doing tree-wide changes can be a real pain.
> 
> Additionally, you may have missed earlier discussions on the linux-scsi
> mailing list about this driver. The first time it was suggested to remove
> this driver was several years ago. The outcome of a discussion of a few
> weeks ago is that there is agreement about the removal of this driver. See
> also the following messages:
> * https://www.spinics.net/lists/linux-scsi/msg123738.html
> * https://www.spinics.net/lists/linux-scsi/msg123742.html
> 
> Bart.

Re: [PATCH] libosd: Remove ignored __weak attribute

[Martin K. Petersen]

Bart,

> Removing kernel drivers that are not used helps to reduce the workload
> of a maintainer and hence is a rational action. Additionally, if
> anyone would ever complain about removal of a kernel driver, it can be
> brought back by reverting the commit through which it has been
> removed.  Martin, please reply if you see this differently.

We remove crusty old SCSI drivers all the time. The heuristic is based
on lack of user bug reports and absence of commits that are not due to
kernel interface changes or trivial cleanups. So removing stuff is
perfectly normal.

The OSD protocol failed to get traction in the industry, adoption was
very limited. If the code just plugged straight into existing kernel
interfaces it would be easier to justify keeping it around. However, the
OSD support requires bidirectional command support so we carry a bunch
of additional plumbing in both block and SCSI to accommodate it. There
are no other users of these interfaces, so dropping OSD would mean we
could simplify some (hot) code paths. That would be a win in my book.
Consequently, if a patch were to materialize that disentangled and
removed OSD, I'd be inclined to merge it.

But I do think that this is an orthogonal discussion to the innocuous
__weak attribute cleanup.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] libosd: Remove ignored __weak attribute

[Christoph Hellwig]

On Sat, Oct 27, 2018 at 09:28:21AM -0400, Martin K. Petersen wrote:
> The OSD protocol failed to get traction in the industry, adoption was
> very limited. If the code just plugged straight into existing kernel
> interfaces it would be easier to justify keeping it around. However, the
> OSD support requires bidirectional command support so we carry a bunch
> of additional plumbing in both block and SCSI to accommodate it. There
> are no other users of these interfaces, so dropping OSD would mean we
> could simplify some (hot) code paths. That would be a win in my book.
> Consequently, if a patch were to materialize that disentangled and
> removed OSD, I'd be inclined to merge it.

In addition to the exofs and osd removal I sent out I've also done the
SCSI cleanup here:

http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/remove-scsi-osd

unfortunately the bsg-lib code also uses the block bidi support, but
then again at least for the blk-mq case that code isn't too bad,
and Jens is about to remove the legacy request code.

Re: [PATCH] libosd: Remove ignored __weak attribute

[Boaz Harrosh]

On 27/10/18 16:28, Martin K. Petersen wrote:
> 
> Bart,
> 
>> Removing kernel drivers that are not used helps to reduce the workload
>> of a maintainer and hence is a rational action. Additionally, if
>> anyone would ever complain about removal of a kernel driver, it can be
>> brought back by reverting the commit through which it has been
>> removed.  Martin, please reply if you see this differently.
> 
> We remove crusty old SCSI drivers all the time. The heuristic is based
> on lack of user bug reports and absence of commits that are not due to
> kernel interface changes or trivial cleanups. So removing stuff is
> perfectly normal.
> 
> The OSD protocol failed to get traction in the industry, adoption was
> very limited. 

Very true.

> If the code just plugged straight into existing kernel
> interfaces it would be easier to justify keeping it around. 

It was using the very much straight existing kernel interfaces
at the time of its insertion, basically the regular PC_command
(As opposed to FS_commnnd) Where the CDB/scsi_command content is
provided by the caller. And the commands complete as an whole.
Plus the added support for bidi PC_commands.

Please tell me what are those entangled interfaces you are unhappy with?
I would like to help disentangle them.

> However, the
> OSD support requires bidirectional command support so we carry a bunch
> of additional plumbing in both block and SCSI to accommodate it. There
> are no other users of these interfaces, so dropping OSD would mean we
> could simplify some (hot) code paths. That would be a win in my book.

But OSD is not the only one or scsi command-set that is using bidi. There
are a few more uses of scsi and BTW block-layer bidi commands in the field.
Target drivers been supporting bidi commands. Vendor drivers use bidi interface
for management CDBS. I'm afraid that by the Linus rules you cannot remove bidi.
Orthogonal to t10-osd or not to be.

> Consequently, if a patch were to materialize that disentangled and
> removed OSD, I'd be inclined to merge it.
> 

Is there a way I can help with this?

> But I do think that this is an orthogonal discussion to the innocuous
> __weak attribute cleanup.
> 

Thanks
Boaz

Re: [PATCH] libosd: Remove ignored __weak attribute

[Boaz Harrosh]

On 26/10/18 20:54, Nick Desaulniers wrote:
<>
> 
> It's hard to say without knowing the original intent of the code.
>>From the variable's identifier and fact that it's global, I *assume*
> that we want only 1 struct osd_obj_id which is the root, hence the
> identifier `osd_root_object`.  It has 4 references in the entire
> kernel; it doesn't make sense to my why those references would want to
> be referring to two different instances of `osd_root_object`.  Maybe
> the maintainers can clarify if 2 instances is the intent?
> 
> Further complicated is the use of the __weak attribute AND the
> compiler flag -fno-common (which the kernel sets in the top level
> Makefile).  Also, it seems that ODR is a C++ concept; it's not clear
> to me if there's semantic differences with C or not (I don't think so
> in this case, but I've learned not to bet on subtle semantic
> differences between the languages not existing).
> 
> __attribute__((weak)) AND static on a global variable declared in a
> header raises many red flags to me.  Was weak added to work around an
> ODR link error?
> 
> If creating one instance of this variable is a functional change, I
> can't help but suspect the original code was wrong.  But maybe Bart,
> Boaz, or Christoph can clarify or have more thoughts on this?  Looks
> like Boaz added this header in commit de258bf5e638 ("[SCSI] libosd:
> OSDv1 Headers").
> 

Sorry for the late response. Was off line for a bit.

The original patch and all its permutations are all correct
the definition of osd_root_object is just a fancy type cast of
couple of zeros. IE a couple of zeroes with the right type. So they
can be simply compared to retuned and sent line content. So it does
not matter at all what change is accepted.

I'm so sorry for your trouble and test development. It could all
be saved if I was noticing this thread.

I will ACK again the original simple V2 patch.

Thanks
Boaz

<>

Re: [PATCH] libosd: Remove ignored __weak attribute

[Boaz Harrosh]

On 26/10/18 00:31, Nathan Chancellor wrote:
> On Tue, Oct 02, 2018 at 04:06:31PM -0700, Bart Van Assche wrote:
<>
> 
> Hi Bart,
> 
> I'm sorry if I didn't follow the conclusion of this conversation properly
> but this is the below diff you were initially looking for, correct?
> 
> If so, Boaz and Nick, do you have any objections if this is v2? I'd like
> to get this patch accepted so the warning can be fixed for everyone.
> 

ACK on both the original and below code they will all work just fine.

I do like the original "just remove the _weak thingy". But this one
will work as well.

The "with extern" version suggested before is more cumbersome because it will
need an EXPORT_SYMBOL() but will also work.

The all use of osd_root_object is just. A properly C-types pointer to couple
of ZEROS. but since the Interface needs a pointer, those zeros need storage
somewhere. It does not matter where. (The value of the pointer is not used
only its content)

Do you need that I send a proper patch? Actually the original first patch
is the version I like. (And again all 3 approaches will work)

Thanks
Boaz

> Thanks,
> Nathan
> 
> ================================================================================
> 
> diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c
> index e19fa883376f..4250f739beb3 100644
> --- a/drivers/scsi/osd/osd_initiator.c
> +++ b/drivers/scsi/osd/osd_initiator.c
> @@ -58,6 +58,8 @@
> 
>  enum { OSD_REQ_RETRIES = 1 };
> 
> +static const struct osd_obj_id osd_root_object;
> +
>  MODULE_AUTHOR("Boaz Harrosh <ooo@electrozaur.com>");
>  MODULE_DESCRIPTION("open-osd initiator library libosd.ko");
>  MODULE_LICENSE("GPL");
> diff --git a/drivers/scsi/osd/osd_uld.c b/drivers/scsi/osd/osd_uld.c
> index eaf36ccf58db..770c758baaa9 100644
> --- a/drivers/scsi/osd/osd_uld.c
> +++ b/drivers/scsi/osd/osd_uld.c
> @@ -73,6 +73,7 @@
> 
>  static const char osd_name[] = "osd";
>  static const char *osd_version_string = "open-osd 0.2.1";
> +static const struct osd_obj_id osd_root_object;
> 
>  MODULE_AUTHOR("Boaz Harrosh <ooo@electrozaur.com>");
>  MODULE_DESCRIPTION("open-osd Upper-Layer-Driver osd.ko");
> diff --git a/include/scsi/osd_types.h b/include/scsi/osd_types.h
> index 48e8a165e136..eb31357ec8b3 100644
> --- a/include/scsi/osd_types.h
> +++ b/include/scsi/osd_types.h
> @@ -28,8 +28,6 @@ struct osd_obj_id {
>         osd_id id;
>  };
> 
> -static const struct __weak osd_obj_id osd_root_object = {0, 0};
> -
>  struct osd_attr {
>         u32 attr_page;
>         u32 attr_id;
> 

Re: [PATCH] libosd: Remove ignored __weak attribute

[Nathan Chancellor]

On Thu, Nov 01, 2018 at 03:39:54AM +0200, Boaz Harrosh wrote:
> On 26/10/18 00:31, Nathan Chancellor wrote:
> > On Tue, Oct 02, 2018 at 04:06:31PM -0700, Bart Van Assche wrote:
> <>
> > 
> > Hi Bart,
> > 
> > I'm sorry if I didn't follow the conclusion of this conversation properly
> > but this is the below diff you were initially looking for, correct?
> > 
> > If so, Boaz and Nick, do you have any objections if this is v2? I'd like
> > to get this patch accepted so the warning can be fixed for everyone.
> > 
> 
> ACK on both the original and below code they will all work just fine.
> 
> I do like the original "just remove the _weak thingy". But this one
> will work as well.
> 
> The "with extern" version suggested before is more cumbersome because it will
> need an EXPORT_SYMBOL() but will also work.
> 
> The all use of osd_root_object is just. A properly C-types pointer to couple
> of ZEROS. but since the Interface needs a pointer, those zeros need storage
> somewhere. It does not matter where. (The value of the pointer is not used
> only its content)
> 
> Do you need that I send a proper patch? Actually the original first patch
> is the version I like. (And again all 3 approaches will work)
> 
> Thanks
> Boaz
> 

Hi Boaz,

I am fine with the original v1 as long as you and everyone else are.
I don't mind resending or making a v2 if I need to as well. Just would
like everyone to come to an agreement on something!

Thanks for the review and clarity,
Nathan

> > Thanks,
> > Nathan
> > 
> > ================================================================================
> > 
> > diff --git a/drivers/scsi/osd/osd_initiator.c b/drivers/scsi/osd/osd_initiator.c
> > index e19fa883376f..4250f739beb3 100644
> > --- a/drivers/scsi/osd/osd_initiator.c
> > +++ b/drivers/scsi/osd/osd_initiator.c
> > @@ -58,6 +58,8 @@
> > 
> >  enum { OSD_REQ_RETRIES = 1 };
> > 
> > +static const struct osd_obj_id osd_root_object;
> > +
> >  MODULE_AUTHOR("Boaz Harrosh <ooo@electrozaur.com>");
> >  MODULE_DESCRIPTION("open-osd initiator library libosd.ko");
> >  MODULE_LICENSE("GPL");
> > diff --git a/drivers/scsi/osd/osd_uld.c b/drivers/scsi/osd/osd_uld.c
> > index eaf36ccf58db..770c758baaa9 100644
> > --- a/drivers/scsi/osd/osd_uld.c
> > +++ b/drivers/scsi/osd/osd_uld.c
> > @@ -73,6 +73,7 @@
> > 
> >  static const char osd_name[] = "osd";
> >  static const char *osd_version_string = "open-osd 0.2.1";
> > +static const struct osd_obj_id osd_root_object;
> > 
> >  MODULE_AUTHOR("Boaz Harrosh <ooo@electrozaur.com>");
> >  MODULE_DESCRIPTION("open-osd Upper-Layer-Driver osd.ko");
> > diff --git a/include/scsi/osd_types.h b/include/scsi/osd_types.h
> > index 48e8a165e136..eb31357ec8b3 100644
> > --- a/include/scsi/osd_types.h
> > +++ b/include/scsi/osd_types.h
> > @@ -28,8 +28,6 @@ struct osd_obj_id {
> >         osd_id id;
> >  };
> > 
> > -static const struct __weak osd_obj_id osd_root_object = {0, 0};
> > -
> >  struct osd_attr {
> >         u32 attr_page;
> >         u32 attr_id;
> > 
> 

[PATCH RESEND] libosd: Remove ignored __weak attribute

[Nathan Chancellor]

Clang warns that the __weak attribute is going to be ignored on
osd_root_object because it's not in the correct location (needs to be
after the type).

./include/scsi/osd_types.h:31:21: warning: 'weak' attribute only applies
to variables, functions, and classes [-Wignored-attributes]
static const struct __weak osd_obj_id osd_root_object = {0, 0};
                    ^

Turns out that GCC ignores the attribute too albeit silently because
moving the attribute after either osd_obj_id or osd_root_object like
all other uses of __weak on variables in the kernel causes a build
error on both GCC and Clang because static variables cannot be weak
since weak definitions rely on not having internal linkage:

./include/scsi/osd_types.h:31:32: error: weak declaration cannot have
internal linkage
static const struct osd_obj_id __weak osd_root_object = {0, 0};
                               ^

Just remove the attribute because it hasn't been correct since the
initial addition of this file in commit de258bf5e638 ("[SCSI] libosd:
OSDv1 Headers").

Reported-by: Nick Desaulniers <ndesaulniers@google.com>
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---

Resending as there was no conclusion on the removal of the OSD code and
this warning is still present on mainline/next. I think it is rather
unfair for this patch to be held back because of the possibility of
removing this code when it is a very simple clean up patch and doesn't
affect anything other than a build time warning. If this declaration or
driver needs to be cleaned up in other ways, it should be done in
subsequent patches.

Thanks,
Nathan

 include/scsi/osd_types.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/scsi/osd_types.h b/include/scsi/osd_types.h
index 48e8a165e136..6b6fdcafa6cc 100644
--- a/include/scsi/osd_types.h
+++ b/include/scsi/osd_types.h
@@ -28,7 +28,7 @@ struct osd_obj_id {
 	osd_id id;
 };
 
-static const struct __weak osd_obj_id osd_root_object = {0, 0};
+static const struct osd_obj_id osd_root_object = {0, 0};
 
 struct osd_attr {
 	u32 attr_page;
-- 
2.20.1