| Message ID | 20180930205448.26205-1-natechancellor@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | libosd: Remove ignored __weak attribute | expand |
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.
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.
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
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.
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
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.
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.
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.
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.
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;
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?
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
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
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.
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
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
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
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.
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
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").
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
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.
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."
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.
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?
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.
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
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.
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.
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.
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.
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
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 <>
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; >
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; > > >
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;
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(-)