libosd: Remove ignored __weak attribute

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 <[email protected]>

Reviewed-by Nick Desaulniers <[email protected]>

Signed-off-by Nathan Chancellor <[email protected]>

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

include/scsi/osd_types.h +1 -2

index 48e8a165e136..6b6fdcafa6cc 100644
@@ -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

include/scsi/osd_types.h struct osd_obj_id { · line 30

Quoted 9 lines · Nathan Chancellor

include/scsi/osd_types.h +1 -1

index 48e8a165e136..6b6fdcafa6cc 100644
@@ -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.

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?

Quoted 4 lines · Bart Van Assche

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.

include/scsi/osd_types.h struct osd_obj_id { · line 31

Quoted 10 lines · Nathan Chancellor

include/scsi/osd_types.h +1 -1

index 48e8a165e136..6b6fdcafa6cc 100644
@@ -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.

Quoted 2 lines · Bart Van Assche

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 nmsays 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?

From https://en.wikipedia.org/wiki/.bss:

Quoted 4 lines · Nick Desaulniers

Quoted 2 lines · Bart Van Assche

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

Quoted 9 lines

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

Quoted 2 lines · Bart Van Assche

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?

Quoted 2 lines

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

Quoted 24 lines · Nick Desaulniers

Quoted 2 lines · Bart Van Assche

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?

Quoted 2 lines

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

Quoted 1 line

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.

Quoted 25 lines · Bart Van Assche

Quoted 24 lines · Nick Desaulniers

Quoted 2 lines · Bart Van Assche

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?

Quoted 2 lines

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

Quoted 7 lines

Quoted 1 line

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

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

drivers/scsi/osd/osd_initiator.c +0 -0

index e19fa883376f..4250f739beb3 100644

Quoted 35 lines · Nathan Chancellor

Quoted 25 lines · Bart Van Assche

Quoted 24 lines · Nick Desaulniers

Quoted 2 lines · Bart Van Assche

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?

Quoted 2 lines

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

Quoted 7 lines

Quoted 1 line

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.

Quoted 7 lines

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.

Quoted 11 lines

Thanks,
Nathan

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

drivers/scsi/osd/osd_initiator.c +0 -0

index e19fa883376f..4250f739beb3 100644

drivers/scsi/osd/osd_initiator.c line 61

Quoted 4 lines

drivers/scsi/osd/osd_initiator.c +1 -0

@@ -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;

drivers/scsi/osd/osd_uld.c line 76

Quoted 13 lines

drivers/scsi/osd/osd_initiator.c +1 -0

+
 MODULE_AUTHOR("Boaz Harrosh <[email protected]>");
 MODULE_DESCRIPTION("open-osd initiator library libosd.ko");
 MODULE_LICENSE("GPL");

drivers/scsi/osd/osd_uld.c +1 -0

index eaf36ccf58db..770c758baaa9 100644
@@ -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;

include/scsi/osd_types.h struct osd_obj_id { · line 32

Quoted 13 lines

drivers/scsi/osd/osd_uld.c +0 -0

 MODULE_AUTHOR("Boaz Harrosh <[email protected]>");
 MODULE_DESCRIPTION("open-osd Upper-Layer-Driver osd.ko");

include/scsi/osd_types.h +0 -2

index 48e8a165e136..eb31357ec8b3 100644
@@ -28,8 +28,6 @@ struct osd_obj_id {
        osd_id id;
 };
 
-static const struct __weak osd_obj_id osd_root_object = {0, 0};
-

LGTM

include/scsi/osd_types.h struct osd_obj_id { · line 33

Quoted 3 lines

include/scsi/osd_types.h +0 -0

 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?

Signature · 2 lines

Thanks,
~Nick Desaulniers

Quoted 45 lines · Nick Desaulniers

Quoted 35 lines · Nathan Chancellor

Quoted 25 lines · Bart Van Assche

Quoted 24 lines · Nick Desaulniers

Quoted 2 lines · Bart Van Assche

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?

Quoted 2 lines

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

Quoted 7 lines

Quoted 1 line

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.

Quoted 7 lines

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.

Quoted 8 lines

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?

From the variable's identifier and fact that it's global, I *assume*

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

Quoted 65 lines

Quoted 11 lines

Thanks,
Nathan

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

drivers/scsi/osd/osd_initiator.c +0 -0

index e19fa883376f..4250f739beb3 100644

drivers/scsi/osd/osd_initiator.c line 61

Quoted 4 lines

drivers/scsi/osd/osd_initiator.c +1 -0

@@ -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;

drivers/scsi/osd/osd_uld.c line 76

Quoted 13 lines

drivers/scsi/osd/osd_initiator.c +1 -0

+
 MODULE_AUTHOR("Boaz Harrosh <[email protected]>");
 MODULE_DESCRIPTION("open-osd initiator library libosd.ko");
 MODULE_LICENSE("GPL");

drivers/scsi/osd/osd_uld.c +1 -0

index eaf36ccf58db..770c758baaa9 100644
@@ -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;

include/scsi/osd_types.h struct osd_obj_id { · line 32

Quoted 13 lines

drivers/scsi/osd/osd_uld.c +0 -0

 MODULE_AUTHOR("Boaz Harrosh <[email protected]>");
 MODULE_DESCRIPTION("open-osd Upper-Layer-Driver osd.ko");

include/scsi/osd_types.h +0 -2

index 48e8a165e136..eb31357ec8b3 100644
@@ -28,8 +28,6 @@ struct osd_obj_id {
        osd_id id;
 };
 
-static const struct __weak osd_obj_id osd_root_object = {0, 0};
-

LGTM

include/scsi/osd_types.h struct osd_obj_id { · line 33

Quoted 3 lines

include/scsi/osd_types.h +0 -0

 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

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

Quoted 5 lines · Nick Desaulniers

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.

Quoted 7 lines · Bart Van Assche

Quoted 5 lines · Nick Desaulniers

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

Quoted 8 lines

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

+ Linus
I'm not about the process of removing code from the kernel. Doesn't that violate the "thou shalt not break userspace" rule?

Quoted 4 lines · Nick Desaulniers

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

Is removing a filesystem considered a userspace breakage?

Quoted 2 lines · Nick Desaulniers

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

Quoted 5 lines · Nick Desaulniers

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

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

Quoted 10 lines · Bart Van Assche

Quoted 5 lines · Nick Desaulniers

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,

Quoted 2 lines

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

Quoted 11 lines · Nick Desaulniers

Quoted 10 lines · Bart Van Assche

Quoted 5 lines · Nick Desaulniers

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,

Quoted 7 lines

Quoted 2 lines

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?

Quoted 23 lines · Bart Van Assche

Quoted 11 lines · Nick Desaulniers

Quoted 10 lines · Bart Van Assche

Quoted 5 lines · Nick Desaulniers

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,

Quoted 7 lines

Quoted 2 lines

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,

Quoted 3 lines

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

Quoted 31 lines · Nick Desaulniers

Quoted 21 lines · Bart Van Assche

Quoted 11 lines · Nick Desaulniers

Quoted 10 lines · Bart Van Assche

Quoted 5 lines · Nick Desaulniers

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,

Quoted 7 lines

Quoted 2 lines

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

Quoted 5 lines

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.

Quoted 1 line

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.

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?

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.

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.

Bart,

Quoted 5 lines

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.

Quoted 4 lines

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?

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.

Quoted 3 lines · Theodore Y. Ts'o

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.

Quoted 3 lines

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.

It's hard to say without knowing the original intent of the code.

Quoted 1 line

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

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.

Thanks,
Nathan

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

drivers/scsi/osd/osd_initiator.c +2 -0

index e19fa883376f..4250f739beb3 100644
@@ -58,6 +58,8 @@
 
 enum { OSD_REQ_RETRIES = 1 };
 
+static const struct osd_obj_id osd_root_object;
+
 MODULE_AUTHOR("Boaz Harrosh <[email protected]>");
 MODULE_DESCRIPTION("open-osd initiator library libosd.ko");
 MODULE_LICENSE("GPL");

drivers/scsi/osd/osd_uld.c +1 -0

index eaf36ccf58db..770c758baaa9 100644
@@ -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 <[email protected]>");
 MODULE_DESCRIPTION("open-osd Upper-Layer-Driver osd.ko");

include/scsi/osd_types.h +0 -2

index 48e8a165e136..eb31357ec8b3 100644
@@ -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;

Quoted 1 line · Nathan Chancellor

On Tue, Oct 02, 2018 at 04:06:31PM -0700, Bart Van Assche wrote:

<>

Quoted 9 lines

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

Quoted 44 lines

Thanks,
Nathan

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

drivers/scsi/osd/osd_initiator.c +2 -0

index e19fa883376f..4250f739beb3 100644
@@ -58,6 +58,8 @@
 
 enum { OSD_REQ_RETRIES = 1 };
 
+static const struct osd_obj_id osd_root_object;
+
 MODULE_AUTHOR("Boaz Harrosh <[email protected]>");
 MODULE_DESCRIPTION("open-osd initiator library libosd.ko");
 MODULE_LICENSE("GPL");

drivers/scsi/osd/osd_uld.c +1 -0

index eaf36ccf58db..770c758baaa9 100644
@@ -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 <[email protected]>");
 MODULE_DESCRIPTION("open-osd Upper-Layer-Driver osd.ko");

include/scsi/osd_types.h +0 -2

index 48e8a165e136..eb31357ec8b3 100644
@@ -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;