Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rbd-nbd: warn when kernel parameters are ignored #13694

Merged
merged 2 commits into from Mar 11, 2017
Merged

rbd-nbd: warn when kernel parameters are ignored #13694

merged 2 commits into from Mar 11, 2017

Conversation

liupan1111
Copy link
Contributor

@liupan1111 liupan1111 commented Feb 28, 2017

…oaded.

when user specify --nbds_max, nbd-rbd will try to load nbd module, and set parm nbds_max. But if the nbd module is already loaded, new nbds_max would work. Prompt message should be dumped.

Fixes: http://tracker.ceph.com/issues/19108

Signed-off-by: Pan Liu liupan1111@gmail.com

@liupan1111
Copy link
Contributor Author

@trociny and @dillaman , please help take a look

}
nbd = open(path, O_RDWR);
}

if (nbds_max && try_load_module && !load_module) {
cerr << "rbd-nbd: nbds_max param doesn't work, because nbd module already loaded." << std::endl;
Copy link
Contributor

@trociny trociny Feb 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@liupan1111

I would prefer a message like "rbd-nbd: ignoring nbds_max option: nbd module already loaded" or "rbd-nbd: nbd module already loaded, ignoring nbds_max option". Or may be Jason will have a better suggestion as a native speaker.

Also, don't we have to do the same for max_part?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@liupan1111 @trociny How about something simple like "rbd-nbd: ignoring kernel module parameter options: nbd module already loaded" if either nbds_max or max_part are specified and the module is already open.

Also, can you open a bug tracker ticket for this PR?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dillaman Sure, I will create a bug tracker ticket.
@trociny , max_part makes me dilemma, it is different from nbds_max:
the default value in our rbd-nbd is 0, and if(nbds_max) is false if the user not specify nbds_max, however, the default value of max_part is 255(in order to support max partition), and if(max_part) is true, even if the user not specify it...

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@liupan1111 You can use a bool to track whether or not the user specified "--max_part" on the CLI

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool!

}
nbd = open(path, O_RDWR);
}

if ((nbds_max || set_max_part) && try_load_module && !load_module) {
cerr << "rbd-nbd: ignoring kernel module parameter options: nbd module already loaded." << std::endl;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: could you consider breaking the line not to exceed 80 char length? Also the final '.' is not needed. Otherwise lgtm

@@ -452,6 +453,7 @@ class NBDWatchCtx : public librbd::UpdateWatchCtx
static int open_device(const char* path, bool try_load_module = false)
{
int nbd = open(path, O_RDWR);
bool load_module = false;
Copy link

@dillaman dillaman Feb 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: module_loaded or loaded_module probably better name given its use below

@liupan1111
Copy link
Contributor Author

Modified, please take a look, thanks.

@trociny
Copy link
Contributor

trociny commented Mar 1, 2017

@ceph-jenkins retest this please

@liupan1111
Copy link
Contributor Author

@trociny haha, it passed this time

Copy link

@dillaman dillaman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@dillaman dillaman changed the title rbd-nbd: prompt message when input nbds_max, and nbd module already l… rbd-nbd: warn when kernel parameters are ignored Mar 1, 2017
@dillaman dillaman added cleanup and removed bug-fix labels Mar 1, 2017
@trociny
Copy link
Contributor

trociny commented Mar 1, 2017

Sorry, I should have done this earlier. Anyway, when I tested this manually, I noticed that the message may repeat several times if you have several devices already mapped:

alfa:~/ceph.ci/build% sudo ./bin/rbd-nbd --nbds_max 255 list-mapped
/dev/nbd0
/dev/nbd1
/dev/nbd2
alfa:~/ceph.ci/build% sudo ./bin/rbd-nbd --nbds_max 255 map test4
2017-03-01 14:49:12.421693 7f39d1657a00 -1 WARNING: all dangerous and experimental features are enabled.
2017-03-01 14:49:12.421833 7f39d1657a00 -1 WARNING: all dangerous and experimental features are enabled.
rbd-nbd: ignoring kernel module parameter options: nbd module already loaded
rbd-nbd: ignoring kernel module parameter options: nbd module already loaded
rbd-nbd: ignoring kernel module parameter options: nbd module already loaded
rbd-nbd: ignoring kernel module parameter options: nbd module already loaded
/dev/nbd3

This is because when a devpath is not specified, rbd-nbd iterates over /dev/nbdX starting from 0, until open_device succeeds.

Do we want to improve this? I think we can call open_device with try_load_module = true only on the first try, and use try_load_module = false for all consequent calls.

@dillaman
Copy link

dillaman commented Mar 1, 2017

@trociny Nice catch -- definitely only want to print it once

@liupan1111
Copy link
Contributor Author

right, i will fix it

@dillaman
Copy link

dillaman commented Mar 7, 2017

@liupan1111 ping

@liupan1111
Copy link
Contributor Author

new version, please take a look.

@@ -452,6 +453,9 @@ class NBDWatchCtx : public librbd::UpdateWatchCtx
static int open_device(const char* path, bool try_load_module = false)
{
int nbd = open(path, O_RDWR);
bool loaded_module = false;
static bool printed_ignore_info = false;
Copy link
Contributor

@trociny trociny Mar 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@liupan1111 I was proposing to call open_device() with try_load_module = true only once in do_map, i.e. adding a var bool try_load_module = true in do_map, before while loop, pass it to open_device(..., try_load_module) and set to false after the call. Why didn't you like this idea? This looks cleaner and more correct to me (no need to try load module more than one time).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds a good solution, thanks. I will modify it.

@liupan1111
Copy link
Contributor Author

@rhcs-jenkins retest please.

@trociny
Copy link
Contributor

trociny commented Mar 11, 2017

@liupan1111 There is "`" character at the end of "Signed-off-by" line in the log message for your first comment. You might want to clean this. Otherwise LGTM.

BTW, in case you are not aware, you can use -s option when running git commit to automatically add your signature.

@trociny
Copy link
Contributor

trociny commented Mar 11, 2017

@liupan1111 see my previous message. I accidentally addressed it to a wrong person.
@danderson sorry

@liupan1111
Copy link
Contributor Author

@trociny done. git commit -s is really helpful, Thanks!

@trociny
Copy link
Contributor

trociny commented Mar 11, 2017

@liupan1111 There is no "Fixes: http://tracker.ceph.com/issues/19108" in the commit message now.

Pan Liu added 2 commits March 11, 2017 21:48
Fixes: http://tracker.ceph.com/issues/19108
Signed-off-by: Pan Liu <liupan1111@gmail.com>
…g do_map operation.

Signed-off-by: Pan Liu <liupan1111@gmail.com>
@liupan1111
Copy link
Contributor Author

@trociny added, sorry for the trouble...

@trociny trociny merged commit c6b78d4 into ceph:master Mar 11, 2017
@liupan1111 liupan1111 deleted the wip-fix-no-error-map branch March 11, 2017 15:11
@liupan1111
Copy link
Contributor Author

@trociny @dillaman use this place to ask you a question:
After a rbd-nbd map, there will be a daemon process "rbd-nbd map ....". I want to use "gdb -p " to attach it and want to debug. Unfortunately, it hangs there, and only print:
"For bug reporting instructions, please see:
http://www.gnu.org/software/gdb/bugs/.
Attaching to process 31810"

Could you give me some help? Thanks!

@trociny
Copy link
Contributor

trociny commented Mar 14, 2017

@liupan1111 I think this is because the main thread is blocked on a kernel call: ioctl(nbd, NBD_DO_IT) and gdb is waiting for it to return. I don't know a nice way to work with this (actually I don't recall I needed to attach with gdb to a live rbd-nbd process, usually increasing 'rbd debug' level is much more useful), but one possible way is to attach to non-main threads. I.e, get the list of all threads id for the process, running something like below:

ps -eLf |grep rbd-nbd

And attach to a particular rbd-nbd thread (using id from LWP column)

gdb -p $lwp

This way you will need to attach to every thread you are interested in. And don't attach to the main thread! Not sure how helpful it is in your case.

I suppose we could workaround this creating a separate thread for ioctl(nbd, NBD_DO_IT) and waiting on thread join in the main thread. But I have not checked if it works.

@liupan1111
Copy link
Contributor Author

@trociny Thank you for great help. Let me try and let you know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants