track files for checkpointability

This takes a suggestion of Ingo's along with comments from lots of other people. It can track whether a given file is able to be checkpointed. It introduces a f_op to allow easy customization like the reset of the VFS.

This takes a suggestion of Ingo's along with comments from lots of other people. It can track whether a given file is able to be checkpointed. It introduces a f_op to allow easy customization like the reset of the VFS.

From 4002821bf068c122867c3b1819817f0b1fdfc250 Mon Sep 17 00:00:00 2001

---
 fs/proc/base.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

fs/proc/base.c +0 -0

index bde92c3..12543b6 100644

---
 fs/proc/base.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

fs/proc/base.c +0 -0

index bde92c3..12543b6 100644

Quoted 4 lines · Dave Hansen

This takes a suggestion of Ingo's along with comments from lots of other people. It can track whether a given file is able to be checkpointed. It introduces a f_op to allow easy customization like the reset of the VFS.

Here is how alternative looks like
* without touching VFS at all
* without adding default handlers

* without duplicate code every ->checkpoint hook will have

* without largely useless "special file" messages
(what's so special about it?)

* without adding userspace-visible /proc/*/checkpointable

* without recalculating "checkpointable" property on fs_struct on every C/R=y kernel.

* with "ban by default" policy as well
* with error message immediatly understandable by developer:

	cr_check_file: can't checkpoint file f61a0f40, ->f_op = socket_file_ops+0x0/0x1c0

It may lack some printk, but printks are trivial to insert including using d_path for precise info.

* without largely useless "special file" messages
(what's so special about it?)

* without largely useless "special file" messages
(what's so special about it?)

Quoted 6 lines · Alexey Dobriyan

Quoted 4 lines · Dave Hansen

This takes a suggestion of Ingo's along with comments from lots of other people. It can track whether a given file is able to be checkpointed. It introduces a f_op to allow easy customization like the reset of the VFS.

Quoted 3 lines

Here is how alternative looks like
* without touching VFS at all
* without adding default handlers

Are these bad things? If this was harmful to the VFS, I can bet Christoph would be speaking up. As far as the default handlers, blame Christoph. :)

Quoted 1 line

* without duplicate code every ->checkpoint hook will have

Well, I had actually planned to break the generic function up into a "common" function that all of the handlers can call or could be called before each handler. This is trivially fixable, but it would look kinda goofy without some code to use it.

Quoted 2 lines

* without largely useless "special file" messages
(what's so special about it?)

Very true. I'll improve that one.

Quoted 1 line

* without adding userspace-visible /proc/*/checkpointable

Ingo, could you weigh in on how you expected this "checkpointable" flag to get exposed to and checked by userspace?

Quoted 2 lines

* without recalculating "checkpointable" property on fs_struct on every C/R=y kernel.

Yeah, this is certainly less than ideal. Although, I haven't seen your proposal for where to tie your code into the kernel. Do you suggest that we do nothing during normal kernel runtime and all the checking at sys_checkpoint() time?

Quoted 2 lines

It may lack some printk, but printks are trivial to insert including using d_path for precise info.

This is definitely workable approach. However, could you show how you would support /dev/null and, say, /proc/$$/stat? I've shown what it takes to do that in my patches, and I think it would show a lot about your approach.

Quoted 8 lines · Dave Hansen

Quoted 2 lines · Alexey Dobriyan

* without recalculating "checkpointable" property on fs_struct on every C/R=y kernel.

Yeah, this is certainly less than ideal. Although, I haven't seen your proposal for where to tie your code into the kernel. Do you suggest that we do nothing during normal kernel runtime and all the checking at sys_checkpoint() time?

Of course!

C/R won't be used by majority of users, so it shouldn't bring any overhead. ->f_op->checkpoint (not ->checkpointable!) is probably acceptable. Recalculating flags is not, sorry.

Imagine, unsupported file is opened between userspace checks for /proc/*/checkpointable and /proc/*/fdinfo/*/checkpointable and whatever, you stil have to do all the checks inside checkpoint(2).

Quoted 7 lines

Quoted 2 lines

It may lack some printk, but printks are trivial to insert including using d_path for precise info.

This is definitely workable approach. However, could you show how you would support /dev/null and, say, /proc/$$/stat? I've shown what it takes to do that in my patches, and I think it would show a lot about your approach.

I haven't yet written code for /dev/null, but it would be:
* at checkpoint(2)

	** see it's block device
	** see it's 1:3 => supported
	** dump "1:3", dump "/dev/null" as filename

* at restore(2)
** read CR_OBJ_FILE
** open filename or -E
** if not block device return -E
** if not 1:3 return -E
** save "struct file *" where needed

(all of this is modulo unlinked case)

Quoted 8 lines · Dave Hansen

Quoted 2 lines · Alexey Dobriyan

* without recalculating "checkpointable" property on fs_struct on every C/R=y kernel.

Yeah, this is certainly less than ideal. Although, I haven't seen your proposal for where to tie your code into the kernel. Do you suggest that we do nothing during normal kernel runtime and all the checking at sys_checkpoint() time?

Of course!

C/R won't be used by majority of users, so it shouldn't bring any overhead. ->f_op->checkpoint (not ->checkpointable!) is probably acceptable. Recalculating flags is not, sorry.

Imagine, unsupported file is opened between userspace checks for /proc/*/checkpointable and /proc/*/fdinfo/*/checkpointable and whatever, you stil have to do all the checks inside checkpoint(2).

Quoted 7 lines

Quoted 2 lines

It may lack some printk, but printks are trivial to insert including using d_path for precise info.

This is definitely workable approach. However, could you show how you would support /dev/null and, say, /proc/$$/stat? I've shown what it takes to do that in my patches, and I think it would show a lot about your approach.

I haven't yet written code for /dev/null, but it would be:
* at checkpoint(2)

	** see it's block device
	** see it's 1:3 => supported
	** dump "1:3", dump "/dev/null" as filename

* at restore(2)
** read CR_OBJ_FILE
** open filename or -E
** if not block device return -E
** if not 1:3 return -E
** save "struct file *" where needed

(all of this is modulo unlinked case)

Quoted 3 lines

Imagine, unsupported file is opened between userspace checks for /proc/*/checkpointable and /proc/*/fdinfo/*/checkpointable and whatever, you stil have to do all the checks inside checkpoint(2).

Alexey, we have two problems here. I completely agree that we have to do complete and thorough checks of each file descriptor at sys_checkpoint(). Any checks made at other times should not be trusted.

The other side is what Ingo has been asking for. How do we *know* when we are checkpointable *before* we call (and without calling)

sys_checkpoint()? You are yet to acknowledge that this is a valid use case, but it is exactly what Ingo is asking for, I believe.

If nice printk()s are sufficient to cover what Ingo wants, I'm quite happy to remove the /proc files.

Quoted 3 lines

Imagine, unsupported file is opened between userspace checks for /proc/*/checkpointable and /proc/*/fdinfo/*/checkpointable and whatever, you stil have to do all the checks inside checkpoint(2).

Alexey, we have two problems here. I completely agree that we have to do complete and thorough checks of each file descriptor at sys_checkpoint(). Any checks made at other times should not be trusted.

The other side is what Ingo has been asking for. How do we *know* when we are checkpointable *before* we call (and without calling)

sys_checkpoint()? You are yet to acknowledge that this is a valid use case, but it is exactly what Ingo is asking for, I believe.

If nice printk()s are sufficient to cover what Ingo wants, I'm quite happy to remove the /proc files.

Quoted 8 lines · Dave Hansen

Quoted 3 lines

Imagine, unsupported file is opened between userspace checks for /proc/*/checkpointable and /proc/*/fdinfo/*/checkpointable and whatever, you stil have to do all the checks inside checkpoint(2).

Alexey, we have two problems here. I completely agree that we have to do complete and thorough checks of each file descriptor at sys_checkpoint(). Any checks made at other times should not be trusted.

Quoted 2 lines

The other side is what Ingo has been asking for. How do we *know* when we are checkpointable *before* we call (and without calling)

This "without calling checkpoint(2)" results in much complications as demonstrated.

task_struct and file are not like other structures because they are exposed in /proc.

For PROC_FS=n kernels, one can't even check.

You can do checkpoint(2) without actual dump. You pass, you're most certainly checkpointable (with inevitable race condition in mind).

With time the amount of stuff C/R won't support will approach zero, but the infrastructure for "checkpointable" will stay constant. If it's too much right now, it will be way too much in future.

Quoted 8 lines · Dave Hansen

Quoted 3 lines

Imagine, unsupported file is opened between userspace checks for /proc/*/checkpointable and /proc/*/fdinfo/*/checkpointable and whatever, you stil have to do all the checks inside checkpoint(2).

Alexey, we have two problems here. I completely agree that we have to do complete and thorough checks of each file descriptor at sys_checkpoint(). Any checks made at other times should not be trusted.

Quoted 2 lines

The other side is what Ingo has been asking for. How do we *know* when we are checkpointable *before* we call (and without calling)

This "without calling checkpoint(2)" results in much complications as demonstrated.

task_struct and file are not like other structures because they are exposed in /proc.

For PROC_FS=n kernels, one can't even check.

You can do checkpoint(2) without actual dump. You pass, you're most certainly checkpointable (with inevitable race condition in mind).

With time the amount of stuff C/R won't support will approach zero, but the infrastructure for "checkpointable" will stay constant. If it's too much right now, it will be way too much in future.

Quoted 9 lines · Alexey Dobriyan

Quoted 8 lines · Dave Hansen

Quoted 3 lines

Imagine, unsupported file is opened between userspace checks for /proc/*/checkpointable and /proc/*/fdinfo/*/checkpointable and whatever, you stil have to do all the checks inside checkpoint(2).

Alexey, we have two problems here. I completely agree that we have to do complete and thorough checks of each file descriptor at sys_checkpoint(). Any checks made at other times should not be trusted.

Quoted 5 lines

Quoted 2 lines

The other side is what Ingo has been asking for. How do we *know* when we are checkpointable *before* we call (and without calling)

This "without calling checkpoint(2)" results in much complications as demonstrated.

I'll let you take that up with Ingo. :)

Quoted 2 lines

task_struct and file are not like other structures because they are exposed in /proc.

Very true. But, we can always use the task as a proxy to say whether any of this tasks's *resources* are uncheckpointable. Is this task's ipc_namespace checkpointable, etc...

Quoted 1 line

For PROC_FS=n kernels, one can't even check.

Definitely. I'd be happy to make this check require PROC=y or even DEBUGFS=y. I just want to make the mechanism usable for developers so they're more motivated to find and fix checkpoint issues.

Quoted 2 lines

You can do checkpoint(2) without actual dump. You pass, you're most certainly checkpointable (with inevitable race condition in mind).

OK, so you envision this as maybe calling sys_checkpoint() with a -1 fd or something? I'm generally OK with that. If the /proc stuff is really the sticking point here, I'd be happy to stick it at the end of the series so we can throw it away more easily.

Quoted 3 lines

With time the amount of stuff C/R won't support will approach zero, but the infrastructure for "checkpointable" will stay constant. If it's too much right now, it will be way too much in future.

What have you seen in OpenVZ? Do new things that are not checkpointable pop up very often?

Quoted 9 lines · Alexey Dobriyan

Quoted 8 lines · Dave Hansen

Quoted 3 lines

Imagine, unsupported file is opened between userspace checks for /proc/*/checkpointable and /proc/*/fdinfo/*/checkpointable and whatever, you stil have to do all the checks inside checkpoint(2).

Alexey, we have two problems here. I completely agree that we have to do complete and thorough checks of each file descriptor at sys_checkpoint(). Any checks made at other times should not be trusted.

Quoted 5 lines

Quoted 2 lines

The other side is what Ingo has been asking for. How do we *know* when we are checkpointable *before* we call (and without calling)

This "without calling checkpoint(2)" results in much complications as demonstrated.

I'll let you take that up with Ingo. :)

Quoted 2 lines

task_struct and file are not like other structures because they are exposed in /proc.

Very true. But, we can always use the task as a proxy to say whether any of this tasks's *resources* are uncheckpointable. Is this task's ipc_namespace checkpointable, etc...

Quoted 1 line

For PROC_FS=n kernels, one can't even check.

Definitely. I'd be happy to make this check require PROC=y or even DEBUGFS=y. I just want to make the mechanism usable for developers so they're more motivated to find and fix checkpoint issues.

Quoted 2 lines

You can do checkpoint(2) without actual dump. You pass, you're most certainly checkpointable (with inevitable race condition in mind).

OK, so you envision this as maybe calling sys_checkpoint() with a -1 fd or something? I'm generally OK with that. If the /proc stuff is really the sticking point here, I'd be happy to stick it at the end of the series so we can throw it away more easily.

Quoted 3 lines

With time the amount of stuff C/R won't support will approach zero, but the infrastructure for "checkpointable" will stay constant. If it's too much right now, it will be way too much in future.

What have you seen in OpenVZ? Do new things that are not checkpointable pop up very often?

Quoted 10 lines · Dave Hansen

Quoted 3 lines

Imagine, unsupported file is opened between userspace checks for /proc/*/checkpointable and /proc/*/fdinfo/*/checkpointable and whatever, you stil have to do all the checks inside checkpoint(2).

Alexey, we have two problems here. I completely agree that we have to do complete and thorough checks of each file descriptor at sys_checkpoint(). Any checks made at other times should not be trusted.

The other side is what Ingo has been asking for. How do we *know* when we are checkpointable *before* we call (and without calling)

This "without calling checkpoint(2)" results in much complications as demonstrated.

task_struct and file are not like other structures because they are exposed in /proc. For PROC_FS=n kernels, one can't even check.

You can do checkpoint(2) without actual dump. You pass, you're most certainly checkpointable (with inevitable race condition in mind).

With time the amount of stuff C/R won't support will approach zero, but the infrastructure for "checkpointable" will stay constant. If it's too much right now, it will be way too much in future.

Quoted 2 lines

sys_checkpoint()? You are yet to acknowledge that this is a valid use case, but it is exactly what Ingo is asking for, I believe.

It's a valid requirement.

Quoted 2 lines

If nice printk()s are sufficient to cover what Ingo wants, I'm quite happy to remove the /proc files.

_______________________________________________
Containers mailing list Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
https://lists.linux-foundation.org/mailman/listinfo/containers

Quoted 10 lines · Dave Hansen

Quoted 3 lines

Imagine, unsupported file is opened between userspace checks for /proc/*/checkpointable and /proc/*/fdinfo/*/checkpointable and whatever, you stil have to do all the checks inside checkpoint(2).

Alexey, we have two problems here. I completely agree that we have to do complete and thorough checks of each file descriptor at sys_checkpoint(). Any checks made at other times should not be trusted.

The other side is what Ingo has been asking for. How do we *know* when we are checkpointable *before* we call (and without calling)

This "without calling checkpoint(2)" results in much complications as demonstrated.

task_struct and file are not like other structures because they are exposed in /proc. For PROC_FS=n kernels, one can't even check.

You can do checkpoint(2) without actual dump. You pass, you're most certainly checkpointable (with inevitable race condition in mind).

With time the amount of stuff C/R won't support will approach zero, but the infrastructure for "checkpointable" will stay constant. If it's too much right now, it will be way too much in future.

Quoted 2 lines

sys_checkpoint()? You are yet to acknowledge that this is a valid use case, but it is exactly what Ingo is asking for, I believe.

It's a valid requirement.

Quoted 2 lines

If nice printk()s are sufficient to cover what Ingo wants, I'm quite happy to remove the /proc files.

_______________________________________________
Containers mailing list
[email protected]
https://lists.linux-foundation.org/mailman/listinfo/containers

Quoted 12 lines · Alexey Dobriyan

Quoted 10 lines · Dave Hansen

Quoted 3 lines

Imagine, unsupported file is opened between userspace checks for /proc/*/checkpointable and /proc/*/fdinfo/*/checkpointable and whatever, you stil have to do all the checks inside checkpoint(2).

Alexey, we have two problems here. I completely agree that we have to do complete and thorough checks of each file descriptor at sys_checkpoint(). Any checks made at other times should not be trusted.

The other side is what Ingo has been asking for. How do we *know* when we are checkpointable *before* we call (and without calling)

Quoted 9 lines

This "without calling checkpoint(2)" results in much complications as demonstrated.

task_struct and file are not like other structures because they are exposed in /proc. For PROC_FS=n kernels, one can't even check.

You can do checkpoint(2) without actual dump. You pass, you're most certainly checkpointable (with inevitable race condition in mind).

Ahhh thank you very much Alexey ! I wanted to explain this to Dave a few monthes ago but I failed... probably because of my poor English skills.

https://lists.linux-foundation.org/pipermail/containers/2008-October/013549.html

Why would we add checking all over the place when it MUST be done on the sys_checkpoint() path ? The checkpoint(2) dry-run is definitely the way to go.

Quoted 12 lines · Alexey Dobriyan

Quoted 10 lines · Dave Hansen

Quoted 3 lines

Imagine, unsupported file is opened between userspace checks for /proc/*/checkpointable and /proc/*/fdinfo/*/checkpointable and whatever, you stil have to do all the checks inside checkpoint(2).

Alexey, we have two problems here. I completely agree that we have to do complete and thorough checks of each file descriptor at sys_checkpoint(). Any checks made at other times should not be trusted.

The other side is what Ingo has been asking for. How do we *know* when we are checkpointable *before* we call (and without calling)

Quoted 9 lines

This "without calling checkpoint(2)" results in much complications as demonstrated.

task_struct and file are not like other structures because they are exposed in /proc. For PROC_FS=n kernels, one can't even check.

You can do checkpoint(2) without actual dump. You pass, you're most certainly checkpointable (with inevitable race condition in mind).

Ahhh thank you very much Alexey ! I wanted to explain this to Dave a few monthes ago but I failed... probably because of my poor English skills.

https://lists.linux-foundation.org/pipermail/containers/2008-October/013549.html

Why would we add checking all over the place when it MUST be done on the sys_checkpoint() path ? The checkpoint(2) dry-run is definitely the way to go.

Quoted 4 lines

Quoted 3 lines

With time the amount of stuff C/R won't support will approach zero, but the infrastructure for "checkpointable" will stay constant. If it's too much right now, it will be way too much in future.

Quoted 2 lines

What have you seen in OpenVZ? Do new things that are not checkpointable pop up very often?

Realistically, do you think the uncheckpointable stuff would catch a brand-new unsupported feature? If it has a file interface then I suppose it would. Well, might. I wouldn't be surprised if the authors would cut and paste enough code to paste the .checkpoint = generic_file_checkpoint line :)

Quoted 4 lines

Quoted 3 lines

With time the amount of stuff C/R won't support will approach zero, but the infrastructure for "checkpointable" will stay constant. If it's too much right now, it will be way too much in future.

Quoted 2 lines

What have you seen in OpenVZ? Do new things that are not checkpointable pop up very often?

Realistically, do you think the uncheckpointable stuff would catch a brand-new unsupported feature? If it has a file interface then I suppose it would. Well, might. I wouldn't be surprised if the authors would cut and paste enough code to paste the .checkpoint = generic_file_checkpoint line :)

Quoted 7 lines · Serge E. Hallyn

Quoted 6 lines

Quoted 3 lines

With time the amount of stuff C/R won't support will approach zero, but the infrastructure for "checkpointable" will stay constant. If it's too much right now, it will be way too much in future.

What have you seen in OpenVZ? Do new things that are not checkpointable pop up very often?

Quoted 5 lines

Realistically, do you think the uncheckpointable stuff would catch a brand-new unsupported feature? If it has a file interface then I suppose it would. Well, might. I wouldn't be surprised if the authors would cut and paste enough code to paste the .checkpoint = generic_file_checkpoint line :)

Yeah, that's true. Us maintainers would probably need to keep an eye on that.

Which imo is fine, but my question is whether that leaves any actual value in the persistent per-resource uncheckpointable flag.

Yeah, per resource it should be. That's per task in the normal case - except for threaded workloads where it's shared by threads.

Uncheckpointable should be a one-way flag anyway. We want this to become usable, so uncheckpointable functionality should be as painful as possible, to make sure it's getting fixed ...

Is there any automated test that could discover C/R breakage via brute force? All that matters in such cases is to get the "you broke stuff" information as soon as possible. If it comes at an early stage developers can generally just fix stuff.

Which imo is fine, but my question is whether that leaves any actual value in the persistent per-resource uncheckpointable flag.

Yeah, per resource it should be. That's per task in the normal case - except for threaded workloads where it's shared by threads.

Uncheckpointable should be a one-way flag anyway. We want this to become usable, so uncheckpointable functionality should be as painful as possible, to make sure it's getting fixed ...

Is there any automated test that could discover C/R breakage via brute force? All that matters in such cases is to get the "you broke stuff" information as soon as possible. If it comes at an early stage developers can generally just fix stuff.

Quoted 25 lines · Greg Kurz

Quoted 10 lines · Alexey Dobriyan

Quoted 9 lines · Dave Hansen

Quoted 3 lines

Imagine, unsupported file is opened between userspace checks for /proc/*/checkpointable and /proc/*/fdinfo/*/checkpointable and whatever, you stil have to do all the checks inside checkpoint(2).

Alexey, we have two problems here. I completely agree that we have to do complete and thorough checks of each file descriptor at sys_checkpoint(). Any checks made at other times should not be trusted.

The other side is what Ingo has been asking for. How do we *know* when we are checkpointable *before* we call (and without calling)

Quoted 9 lines

This "without calling checkpoint(2)" results in much complications as demonstrated.

task_struct and file are not like other structures because they are exposed in /proc. For PROC_FS=n kernels, one can't even check.

You can do checkpoint(2) without actual dump. You pass, you're most certainly checkpointable (with inevitable race condition in mind).

Ahhh thank you very much Alexey ! I wanted to explain this to Dave a few monthes ago but I failed... probably because of my poor English skills.

https://lists.linux-foundation.org/pipermail/containers/2008-October/013549.html

Quoted 3 lines

Why would we add checking all over the place when it MUST be done on the sys_checkpoint() path ? The checkpoint(2) dry-run is definitely the way to go.

I'm sure Dave understood that this was possible :)

But what you and Alexey are proposing does not and cannot fullfill Ingo's requirement.

Quoted 2 lines · Serge E. Hallyn

Which imo is fine, but my question is whether that leaves any actual value in the persistent per-resource uncheckpointable flag.

OK, let's take a look back at this discussion a little bit and how we got here.

Ingo quotes:

Quoted 4 lines

Yeah, per resource it should be. That's per task in the normal case - except for threaded workloads where it's shared by threads.

Quoted 8 lines

Uncheckpointable should be a one-way flag anyway. We want this to become usable, so uncheckpointable functionality should be as painful as possible, to make sure it's getting fixed ...

Is there any automated test that could discover C/R breakage via brute force? All that matters in such cases is to get the "you broke stuff" information as soon as possible. If it comes at an early stage developers can generally just fix stuff.

You add these things together and you get what I posted. My patch is:
1. per resource
2. has a one way flag
3. Gives messages to developers at an early stage (dmesg) and lets them explore it more thoroughly (/proc)

But, these "early stage" messages are completely opposed to an approach that uses sys_checkpoint() in some form (like with a -1 fd as an argument).

Think of it like lockdep. We *could* have designed lockdep to simply give us a nice message whenever we do an a/b b/a deadlock. That would be helpful. Or, we could design it to record all lock acquisitions that didn't deadlock to see if they ever possibly deadlock. (We did the second one, btw). That gave an early, useful, warning that developers could fix before we encounter an actual problem. I'm advocating such a mechanism for c/r.

Quoted 2 lines · Serge E. Hallyn

Which imo is fine, but my question is whether that leaves any actual value in the persistent per-resource uncheckpointable flag.

OK, let's take a look back at this discussion a little bit and how we got here.

Ingo quotes:

Quoted 4 lines

Yeah, per resource it should be. That's per task in the normal case - except for threaded workloads where it's shared by threads.

Quoted 8 lines

Uncheckpointable should be a one-way flag anyway. We want this to become usable, so uncheckpointable functionality should be as painful as possible, to make sure it's getting fixed ...

Is there any automated test that could discover C/R breakage via brute force? All that matters in such cases is to get the "you broke stuff" information as soon as possible. If it comes at an early stage developers can generally just fix stuff.

You add these things together and you get what I posted. My patch is:
1. per resource
2. has a one way flag
3. Gives messages to developers at an early stage (dmesg) and lets them explore it more thoroughly (/proc)

But, these "early stage" messages are completely opposed to an approach that uses sys_checkpoint() in some form (like with a -1 fd as an argument).

Think of it like lockdep. We *could* have designed lockdep to simply give us a nice message whenever we do an a/b b/a deadlock. That would be helpful. Or, we could design it to record all lock acquisitions that didn't deadlock to see if they ever possibly deadlock. (We did the second one, btw). That gave an early, useful, warning that developers could fix before we encounter an actual problem. I'm advocating such a mechanism for c/r.

Quoted 30 lines · Serge E. Hallyn

Quoted 28 lines · Greg Kurz

Quoted 10 lines · Alexey Dobriyan

Quoted 9 lines · Dave Hansen

Quoted 3 lines

Imagine, unsupported file is opened between userspace checks for /proc/*/checkpointable and /proc/*/fdinfo/*/checkpointable and whatever, you stil have to do all the checks inside checkpoint(2).

Alexey, we have two problems here. I completely agree that we have to do complete and thorough checks of each file descriptor at sys_checkpoint(). Any checks made at other times should not be trusted.

The other side is what Ingo has been asking for. How do we *know* when we are checkpointable *before* we call (and without calling)

Quoted 9 lines

This "without calling checkpoint(2)" results in much complications as demonstrated.

task_struct and file are not like other structures because they are exposed in /proc. For PROC_FS=n kernels, one can't even check.

You can do checkpoint(2) without actual dump. You pass, you're most certainly checkpointable (with inevitable race condition in mind).

Ahhh thank you very much Alexey ! I wanted to explain this to Dave a few monthes ago but I failed... probably because of my poor English skills.

https://lists.linux-foundation.org/pipermail/containers/2008-October/013549.html

Why would we add checking all over the place when it MUST be done on the sys_checkpoint() path ? The checkpoint(2) dry-run is definitely the way to go.

Quoted 4 lines

I'm sure Dave understood that this was possible :)

But what you and Alexey are proposing does not and cannot fullfill Ingo's requirement.

And if Ingo's requirement is fulfilled, would any C/R patchset be acceptable ?

Quoted 30 lines · Serge E. Hallyn

Quoted 28 lines · Greg Kurz

Quoted 10 lines · Alexey Dobriyan

Quoted 9 lines · Dave Hansen

Quoted 3 lines

Imagine, unsupported file is opened between userspace checks for /proc/*/checkpointable and /proc/*/fdinfo/*/checkpointable and whatever, you stil have to do all the checks inside checkpoint(2).

Alexey, we have two problems here. I completely agree that we have to do complete and thorough checks of each file descriptor at sys_checkpoint(). Any checks made at other times should not be trusted.

The other side is what Ingo has been asking for. How do we *know* when we are checkpointable *before* we call (and without calling)

Quoted 9 lines

This "without calling checkpoint(2)" results in much complications as demonstrated.

task_struct and file are not like other structures because they are exposed in /proc. For PROC_FS=n kernels, one can't even check.

You can do checkpoint(2) without actual dump. You pass, you're most certainly checkpointable (with inevitable race condition in mind).

Ahhh thank you very much Alexey ! I wanted to explain this to Dave a few monthes ago but I failed... probably because of my poor English skills.

https://lists.linux-foundation.org/pipermail/containers/2008-October/013549.html

Why would we add checking all over the place when it MUST be done on the sys_checkpoint() path ? The checkpoint(2) dry-run is definitely the way to go.

Quoted 4 lines

I'm sure Dave understood that this was possible :)

But what you and Alexey are proposing does not and cannot fullfill Ingo's requirement.

And if Ingo's requirement is fulfilled, would any C/R patchset be acceptable ?

Quoted 3 lines

But, these "early stage" messages are completely opposed to an approach that uses sys_checkpoint() in some form (like with a -1 fd as an argument).

Well I disagree with that. The 'early stage' messages could be seen as either:

1. a short-term way to prioritize resources to support or
2. a long-term way to catch new resources introduced without checkpoint/restart support

I don't believe 2. would work. I think 1. would work, but that we risk imposing permanent code changes to support a temporary goal.

Quoted 3 lines

But, these "early stage" messages are completely opposed to an approach that uses sys_checkpoint() in some form (like with a -1 fd as an argument).

Well I disagree with that. The 'early stage' messages could be seen as either:

1. a short-term way to prioritize resources to support or
2. a long-term way to catch new resources introduced without checkpoint/restart support

I don't believe 2. would work. I think 1. would work, but that we risk imposing permanent code changes to support a temporary goal.

Introduce a files_struct counter to indicate whether a particular file_struct has ever contained a file which can not be checkpointed. This flag is a one-way trip; once it is set, it may not be unset.

We assume at allocation that a new files_struct is clean and may be checkpointed. However, as soon as it has had its files filled from its parent's, we check it for real in __scan_files_for_cr(). At that point, we mark it if it contained any uncheckpointable files.

We also check each 'struct file' when it is installed in a fd slot. This way, if anyone open()s or managed to dup() an unsuppored file, we can catch it.

Signed-off-by Dave Hansen <[email protected]>

---

 linux-2.6.git-dave/fs/file.c                  |   19 +++++++++++++++++++
 linux-2.6.git-dave/fs/open.c                  |    5 +++++
 linux-2.6.git-dave/include/linux/checkpoint.h |   13 +++++++++++++
 linux-2.6.git-dave/include/linux/fdtable.h    |    3 +++
 4 files changed, 40 insertions(+)

diff -puN fs/file.c~242c7afafb1a81c0d9a41085536f2197d81146e7 fs/file.c

fs/file.c renamed from fs/file.c~242c7afafb1a81c0d9a41085536f2197d81146e7 +2 -0

@@ -15,6 +15,7 @@
 #include <linux/file.h>
 #include <linux/fdtable.h>
 #include <linux/bitops.h>
+#include <linux/checkpoint.h>
 #include <linux/interrupt.h>
 #include <linux/spinlock.h>
 #include <linux/rcupdate.h>

@@ -285,6 +286,20 @@ static int count_open_files(struct fdtab
 	return i;
 }
 
+static void __scan_files_for_cr(struct files_struct *files)

fs/file.c renamed from fs/file.c~242c7afafb1a81c0d9a41085536f2197d81146e7 +7 -0

+	}
+}
+
 /*
  * Allocate a new files structure and copy contents from the
  * passed in files structure.

@@ -303,6 +318,9 @@ struct files_struct *dup_fd(struct files
 		goto out;
 
 	atomic_set(&newf->count, 1);
+#ifdef CONFIG_CHECKPOINT_RESTART
+	newf->may_checkpoint = 1;
+#endif
 
 	spin_lock_init(&newf->file_lock);
 	newf->next_fd = 0;

@@ -396,6 +414,7 @@ struct files_struct *dup_fd(struct files
 
 	rcu_assign_pointer(newf->fdt, new_fdt);
 
+	__scan_files_for_cr(newf);
 	return newf;
 
 out_release:

diff -puN fs/open.c~242c7afafb1a81c0d9a41085536f2197d81146e7 fs/open.c

fs/open.c renamed from fs/open.c~242c7afafb1a81c0d9a41085536f2197d81146e7 +5 -0

@@ -29,6 +29,7 @@
 #include <linux/rcupdate.h>
 #include <linux/audit.h>
 #include <linux/falloc.h>
+#include <linux/checkpoint.h>
 
 int vfs_statfs(struct dentry *dentry, struct kstatfs *buf)
 {

@@ -1015,6 +1016,10 @@ void fd_install(unsigned int fd, struct 
 {
 	struct files_struct *files = current->files;
 	struct fdtable *fdt;
+
+	if (!cr_file_supported(file))
+		files_deny_checkpointing(files);
+
 	spin_lock(&files->file_lock);
 	fdt = files_fdtable(files);
 	BUG_ON(fdt->fd[fd] != NULL);

diff -puN include/linux/checkpoint.h~242c7afafb1a81c0d9a41085536f2197d81146e7 include/linux/checkpoint.h

include/linux/checkpoint.h renamed from include/linux/checkpoint.h~242c7afafb1a81c0d9a41085536f2197d81146e7 +12 -0

@@ -12,6 +12,7 @@
 
 #include <linux/path.h>
 #include <linux/fs.h>
+#include <linux/fdtable.h>
 
 #ifdef CONFIG_CHECKPOINT_RESTART
 

@@ -101,11 +102,23 @@ extern int cr_read_files(struct cr_ctx *
 
 #define pr_fmt(fmt) "[%d:c/r:%s] " fmt, task_pid_vnr(current), __func__
 
+static inline void __files_deny_checkpointing(struct files_struct *files,
+		char *file, int line)
+{
+	if (!test_and_clear_bit(0, &files->may_checkpoint))
+		return;
+	printk(KERN_INFO "process performed an action that can not be "
+			"checkpointed at: %s:%d\n", file, line);
+}
+#define files_deny_checkpointing(f)  \
+	__files_deny_checkpointing(f, __FILE__, __LINE__)
+

From a0fb96aa41c4d360559013cfd7f32f07f449c1c4 Mon Sep 17 00:00:00 2001

From 3695fbda6225d2436e4af67a4bce6984df0891be Mon Sep 17 00:00:00 2001

Please consider this and a following patch.

Quoted 1 line

From a0fb96aa41c4d360559013cfd7f32f07f449c1c4 Mon Sep 17 00:00:00 2001

From: Nathan Lynch <[email protected]>
Date: Mon, 9 Mar 2009 22:23:02 -0500
Subject: [PATCH] checkpoint: make files_deny_checkpointing print task name and pid

This lets the developer know *which* task performed an action that prevents checkpoint.

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

---
 fs/file.c                  |    2 +-
 fs/open.c                  |    2 +-
 include/linux/checkpoint.h |   13 +++++++------
 3 files changed, 9 insertions(+), 8 deletions(-)

fs/file.c +0 -0

index 0501af6..fcb2803 100644

From a0fb96aa41c4d360559013cfd7f32f07f449c1c4 Mon Sep 17 00:00:00 2001
From: Nathan Lynch <[email protected]>
Date: Mon, 9 Mar 2009 22:23:02 -0500
Subject: [PATCH] checkpoint: make files_deny_checkpointing print task name and pid

This lets the developer know *which* task performed an action that prevents checkpoint.

From a0fb96aa41c4d360559013cfd7f32f07f449c1c4 Mon Sep 17 00:00:00 2001
From: Nathan Lynch <[email protected]>
Date: Mon, 9 Mar 2009 22:23:02 -0500
Subject: [PATCH] checkpoint: make files_deny_checkpointing print task name and pid

This lets the developer know *which* task performed an action that prevents checkpoint.

Quoted 1 line

From 3695fbda6225d2436e4af67a4bce6984df0891be Mon Sep 17 00:00:00 2001

From: Nathan Lynch <[email protected]>
Date: Mon, 9 Mar 2009 22:36:40 -0500
Subject: [PATCH] ratelimit files_deny_checkpointing output

Any common distribution's boot sequence causes thousands of "<something> performed an action that cannot be checkpointed" messages. Kernels are known to produce other messages of interest at times,

so ratelimit the output of files_deny_checkpointing.

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

---
 include/linux/checkpoint.h |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

include/linux/checkpoint.h +4 -2

index 27366ac..efdb8f0 100644
@@ -108,8 +108,10 @@ static inline void __files_deny_checkpointing(const struct task_struct *tsk,
 {
 	if (!test_and_clear_bit(0, &files->may_checkpoint))
 		return;
-	printk(KERN_INFO "%s/%i performed an action that can not be "
-	       "checkpointed at: %s:%d\n", tsk->comm, tsk->pid, file, line);
+	if (printk_ratelimit())
+		printk(KERN_INFO "%s/%i performed an action that can not be "
+				"checkpointed at: %s:%d\n",
+				tsk->comm, tsk->pid, file, line);
 }
 #define files_deny_checkpointing(tsk,f)				\
 	__files_deny_checkpointing(tsk, f, __FILE__, __LINE__)

Signature · 6 lines

1.6.0.6

_______________________________________________
Containers mailing list
Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
https://lists.linux-foundation.org/mailman/listinfo/containers

Please consider this and a following patch.

Quoted 1 line

From a0fb96aa41c4d360559013cfd7f32f07f449c1c4 Mon Sep 17 00:00:00 2001

From: Nathan Lynch <[email protected]>
Date: Mon, 9 Mar 2009 22:23:02 -0500
Subject: [PATCH] checkpoint: make files_deny_checkpointing print task name and pid

This lets the developer know *which* task performed an action that prevents checkpoint.

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

---
 fs/file.c                  |    2 +-
 fs/open.c                  |    2 +-
 include/linux/checkpoint.h |   13 +++++++------
 3 files changed, 9 insertions(+), 8 deletions(-)

fs/file.c +0 -0

index 0501af6..fcb2803 100644

Quoted 1 line

From 3695fbda6225d2436e4af67a4bce6984df0891be Mon Sep 17 00:00:00 2001

From: Nathan Lynch <[email protected]>
Date: Mon, 9 Mar 2009 22:36:40 -0500
Subject: [PATCH] ratelimit files_deny_checkpointing output

Any common distribution's boot sequence causes thousands of "<something> performed an action that cannot be checkpointed" messages. Kernels are known to produce other messages of interest at times,

so ratelimit the output of files_deny_checkpointing.

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

---
 include/linux/checkpoint.h |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

include/linux/checkpoint.h +4 -2

index 27366ac..efdb8f0 100644
@@ -108,8 +108,10 @@ static inline void __files_deny_checkpointing(const struct task_struct *tsk,
 {
 	if (!test_and_clear_bit(0, &files->may_checkpoint))
 		return;
-	printk(KERN_INFO "%s/%i performed an action that can not be "
-	       "checkpointed at: %s:%d\n", tsk->comm, tsk->pid, file, line);
+	if (printk_ratelimit())
+		printk(KERN_INFO "%s/%i performed an action that can not be "
+				"checkpointed at: %s:%d\n",
+				tsk->comm, tsk->pid, file, line);
 }
 #define files_deny_checkpointing(tsk,f)				\
 	__files_deny_checkpointing(tsk, f, __FILE__, __LINE__)

Signature · 6 lines

1.6.0.6

_______________________________________________
Containers mailing list
[email protected]
https://lists.linux-foundation.org/mailman/listinfo/containers

Quoted 18 lines · Nathan Lynch

Please consider this and a following patch.

Quoted 1 line

From a0fb96aa41c4d360559013cfd7f32f07f449c1c4 Mon Sep 17 00:00:00 2001

From: Nathan Lynch <[email protected]>
Date: Mon, 9 Mar 2009 22:23:02 -0500
Subject: [PATCH] checkpoint: make files_deny_checkpointing print task name and pid

This lets the developer know *which* task performed an action that prevents checkpoint.

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

---
 fs/file.c                  |    2 +-
 fs/open.c                  |    2 +-
 include/linux/checkpoint.h |   13 +++++++------
 3 files changed, 9 insertions(+), 8 deletions(-)

fs/file.c static void __scan_files_for_cr(struct files_struct *files) · line 302

Quoted 10 lines

fs/file.c +1 -1

index 0501af6..fcb2803 100644
@@ -299,7 +299,7 @@ static void __scan_files_for_cr(struct files_struct *files)
 			continue;
 		if (cr_file_supported(f))
 			continue;
-		files_deny_checkpointing(files);
+		files_deny_checkpointing(current, files);

Ah but you can't do this, because __scan_files_for_cr is called from dupfd which is called during copy_files, right?

Quoted 18 lines · Nathan Lynch

Please consider this and a following patch.

Quoted 1 line

From a0fb96aa41c4d360559013cfd7f32f07f449c1c4 Mon Sep 17 00:00:00 2001

From: Nathan Lynch <[email protected]>
Date: Mon, 9 Mar 2009 22:23:02 -0500
Subject: [PATCH] checkpoint: make files_deny_checkpointing print task name and pid

This lets the developer know *which* task performed an action that prevents checkpoint.

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

---
 fs/file.c                  |    2 +-
 fs/open.c                  |    2 +-
 include/linux/checkpoint.h |   13 +++++++------
 3 files changed, 9 insertions(+), 8 deletions(-)

fs/file.c static void __scan_files_for_cr(struct files_struct *files) · line 302

Quoted 10 lines

fs/file.c +1 -1

index 0501af6..fcb2803 100644
@@ -299,7 +299,7 @@ static void __scan_files_for_cr(struct files_struct *files)
 			continue;
 		if (cr_file_supported(f))
 			continue;
-		files_deny_checkpointing(files);
+		files_deny_checkpointing(current, files);

Ah but you can't do this, because __scan_files_for_cr is called from dupfd which is called during copy_files, right?

Quoted 19 lines · Serge E. Hallyn

Quoted 18 lines · Nathan Lynch

Please consider this and a following patch.

Quoted 1 line

From a0fb96aa41c4d360559013cfd7f32f07f449c1c4 Mon Sep 17 00:00:00 2001

From: Nathan Lynch <[email protected]>
Date: Mon, 9 Mar 2009 22:23:02 -0500
Subject: [PATCH] checkpoint: make files_deny_checkpointing print task name and pid

This lets the developer know *which* task performed an action that prevents checkpoint.

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

---
 fs/file.c                  |    2 +-
 fs/open.c                  |    2 +-
 include/linux/checkpoint.h |   13 +++++++------
 3 files changed, 9 insertions(+), 8 deletions(-)

Quoted 13 lines

fs/file.c static void __scan_files_for_cr(struct files_struct *files) · line 302

Quoted 10 lines

fs/file.c +1 -1

index 0501af6..fcb2803 100644
@@ -299,7 +299,7 @@ static void __scan_files_for_cr(struct files_struct *files)
 			continue;
 		if (cr_file_supported(f))
 			continue;
-		files_deny_checkpointing(files);
+		files_deny_checkpointing(current, files);

Ah but you can't do this, because __scan_files_for_cr is called from dupfd which is called during copy_files, right?

Are you saying that the message should identify the child instead of the parent as the uncheckpointable task?

Quoted 19 lines · Serge E. Hallyn

Quoted 18 lines · Nathan Lynch

Please consider this and a following patch.

Quoted 1 line

From a0fb96aa41c4d360559013cfd7f32f07f449c1c4 Mon Sep 17 00:00:00 2001

From: Nathan Lynch <[email protected]>
Date: Mon, 9 Mar 2009 22:23:02 -0500
Subject: [PATCH] checkpoint: make files_deny_checkpointing print task name and pid

This lets the developer know *which* task performed an action that prevents checkpoint.

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

---
 fs/file.c                  |    2 +-
 fs/open.c                  |    2 +-
 include/linux/checkpoint.h |   13 +++++++------
 3 files changed, 9 insertions(+), 8 deletions(-)

Quoted 13 lines

fs/file.c static void __scan_files_for_cr(struct files_struct *files) · line 302

Quoted 10 lines

fs/file.c +1 -1

index 0501af6..fcb2803 100644
@@ -299,7 +299,7 @@ static void __scan_files_for_cr(struct files_struct *files)
 			continue;
 		if (cr_file_supported(f))
 			continue;
-		files_deny_checkpointing(files);
+		files_deny_checkpointing(current, files);

Ah but you can't do this, because __scan_files_for_cr is called from dupfd which is called during copy_files, right?

Are you saying that the message should identify the child instead of the parent as the uncheckpointable task?

Quoted 2 lines

Are you saying that the message should identify the child instead of the parent as the uncheckpointable task?

Yes. The parent may have opened the fd (or, importantly, may NOT have) but the child is the one now getting that 'dirty' fd and being newly marked uncheckpointable.

Quoted 2 lines

Are you saying that the message should identify the child instead of the parent as the uncheckpointable task?

Yes. The parent may have opened the fd (or, importantly, may NOT have) but the child is the one now getting that 'dirty' fd and being newly marked uncheckpointable.

Quoted 1 line

And if Ingo's requirement is fulfilled, would any C/R patchset be acceptable ?

Yup, no matter how hideous :) Ok not really.

But the point was that it wasn't Dave not understanding Alexey's suggestion, but Greg not understanding Ingo's. If you think Ingo's goal isn't worthwhile or achievable, then argue that (as I am), don't keep elaborating on something we all agree will be needed (Alexey's suggestion or some other way of doing a true may-be-checkpointed test).

Quoted 1 line

And if Ingo's requirement is fulfilled, would any C/R patchset be acceptable ?

Yup, no matter how hideous :) Ok not really.

But the point was that it wasn't Dave not understanding Alexey's suggestion, but Greg not understanding Ingo's. If you think Ingo's goal isn't worthwhile or achievable, then argue that (as I am), don't keep elaborating on something we all agree will be needed (Alexey's suggestion or some other way of doing a true may-be-checkpointed test).

Quoted 9 lines

Quoted 1 line

And if Ingo's requirement is fulfilled, would any C/R patchset be acceptable ?

Yup, no matter how hideous :) Ok not really.

But the point was that it wasn't Dave not understanding Alexey's suggestion, but Greg not understanding Ingo's. If you think Ingo's goal isn't worthwhile or achievable, then argue that (as I am), don't keep elaborating on something we all agree will be needed (Alexey's suggestion or some other way of doing a true may-be-checkpointed test).

I rather spend my time on enabling things rather than forbid them.

Introduce a files_struct counter to indicate whether a particular file_struct has ever contained a file which can not be checkpointed. This flag is a one-way trip; once it is set, it may not be unset.

We assume at allocation that a new files_struct is clean and may be checkpointed. However, as soon as it has had its files filled from its parent's, we check it for real in __scan_files_for_cr(). At that point, we mark it if it contained any uncheckpointable files.

We also check each 'struct file' when it is installed in a fd slot. This way, if anyone open()s or managed to dup() an unsuppored file, we can catch it.

Signed-off-by Dave Hansen <[email protected]>

---

 linux-2.6.git-dave/fs/file.c                  |   19 +++++++++++++++++++
 linux-2.6.git-dave/fs/open.c                  |    5 +++++
 linux-2.6.git-dave/include/linux/checkpoint.h |   13 +++++++++++++
 linux-2.6.git-dave/include/linux/fdtable.h    |    3 +++
 4 files changed, 40 insertions(+)

diff -puN fs/file.c~242c7afafb1a81c0d9a41085536f2197d81146e7 fs/file.c

fs/file.c renamed from fs/file.c~242c7afafb1a81c0d9a41085536f2197d81146e7 +2 -0

@@ -15,6 +15,7 @@
 #include <linux/file.h>
 #include <linux/fdtable.h>
 #include <linux/bitops.h>
+#include <linux/checkpoint.h>
 #include <linux/interrupt.h>
 #include <linux/spinlock.h>
 #include <linux/rcupdate.h>

@@ -285,6 +286,20 @@ static int count_open_files(struct fdtab
 	return i;
 }
 
+static void __scan_files_for_cr(struct files_struct *files)

fs/file.c renamed from fs/file.c~242c7afafb1a81c0d9a41085536f2197d81146e7 +7 -0

+	}
+}
+
 /*
  * Allocate a new files structure and copy contents from the
  * passed in files structure.

@@ -303,6 +318,9 @@ struct files_struct *dup_fd(struct files
 		goto out;
 
 	atomic_set(&newf->count, 1);
+#ifdef CONFIG_CHECKPOINT_RESTART
+	newf->may_checkpoint = 1;
+#endif
 
 	spin_lock_init(&newf->file_lock);
 	newf->next_fd = 0;

@@ -396,6 +414,7 @@ struct files_struct *dup_fd(struct files
 
 	rcu_assign_pointer(newf->fdt, new_fdt);
 
+	__scan_files_for_cr(newf);
 	return newf;
 
 out_release:

diff -puN fs/open.c~242c7afafb1a81c0d9a41085536f2197d81146e7 fs/open.c

fs/open.c renamed from fs/open.c~242c7afafb1a81c0d9a41085536f2197d81146e7 +5 -0

@@ -29,6 +29,7 @@
 #include <linux/rcupdate.h>
 #include <linux/audit.h>
 #include <linux/falloc.h>
+#include <linux/checkpoint.h>
 
 int vfs_statfs(struct dentry *dentry, struct kstatfs *buf)
 {

@@ -1015,6 +1016,10 @@ void fd_install(unsigned int fd, struct 
 {
 	struct files_struct *files = current->files;
 	struct fdtable *fdt;
+
+	if (!cr_file_supported(file))
+		files_deny_checkpointing(files);
+
 	spin_lock(&files->file_lock);
 	fdt = files_fdtable(files);
 	BUG_ON(fdt->fd[fd] != NULL);

diff -puN include/linux/checkpoint.h~242c7afafb1a81c0d9a41085536f2197d81146e7 include/linux/checkpoint.h

include/linux/checkpoint.h renamed from include/linux/checkpoint.h~242c7afafb1a81c0d9a41085536f2197d81146e7 +12 -0

@@ -12,6 +12,7 @@
 
 #include <linux/path.h>
 #include <linux/fs.h>
+#include <linux/fdtable.h>
 
 #ifdef CONFIG_CHECKPOINT_RESTART
 

@@ -101,11 +102,23 @@ extern int cr_read_files(struct cr_ctx *
 
 #define pr_fmt(fmt) "[%d:c/r:%s] " fmt, task_pid_vnr(current), __func__
 
+static inline void __files_deny_checkpointing(struct files_struct *files,
+		char *file, int line)
+{
+	if (!test_and_clear_bit(0, &files->may_checkpoint))
+		return;
+	printk(KERN_INFO "process performed an action that can not be "
+			"checkpointed at: %s:%d\n", file, line);
+}
+#define files_deny_checkpointing(f)  \
+	__files_deny_checkpointing(f, __FILE__, __LINE__)
+

fs/file.c static int count_open_files(struct fdtab · line 299

Quoted 11 lines · Dave Hansen

fs/file.c renamed from fs/file.c~242c7afafb1a81c0d9a41085536f2197d81146e7 +11 -0

+static void __scan_files_for_cr(struct files_struct *files)
+{
+	int i;
+
+	for (i = 0; i < files->fdtab.max_fds; i++) {
+		struct file *f = fcheck_files(files, i);
+		if (!f)
+			continue;
+		if (cr_file_supported(f))
+			continue;
+		files_deny_checkpointing(files);

At this point couldn't we skip the rest of the loop iterations?

Might it also be useful to print a path to f here? So not only would the log show the location in the kernel source but we'd also get some idea of which file caused the problem? Of course "f" isn't always available everywhere we call files_deny_checkpointing()..

fs/file.c static int count_open_files(struct fdtab · line 299

Quoted 11 lines · Dave Hansen

fs/file.c renamed from fs/file.c~242c7afafb1a81c0d9a41085536f2197d81146e7 +11 -0

+static void __scan_files_for_cr(struct files_struct *files)
+{
+	int i;
+
+	for (i = 0; i < files->fdtab.max_fds; i++) {
+		struct file *f = fcheck_files(files, i);
+		if (!f)
+			continue;
+		if (cr_file_supported(f))
+			continue;
+		files_deny_checkpointing(files);

At this point couldn't we skip the rest of the loop iterations?

Might it also be useful to print a path to f here? So not only would the log show the location in the kernel source but we'd also get some idea of which file caused the problem? Of course "f" isn't always available everywhere we call files_deny_checkpointing()..

This marks ext[234] as being checkpointable. There will be many more to do this to, but this is a start.

Signed-off-by Dave Hansen <[email protected]>

---

 linux-2.6.git-dave/fs/ext2/dir.c  |    1 +
 linux-2.6.git-dave/fs/ext2/file.c |    2 ++
 linux-2.6.git-dave/fs/ext3/dir.c  |    1 +
 linux-2.6.git-dave/fs/ext3/file.c |    1 +
 linux-2.6.git-dave/fs/ext4/dir.c  |    1 +
 linux-2.6.git-dave/fs/ext4/file.c |    1 +
 6 files changed, 7 insertions(+)

diff -puN fs/ext2/dir.c~add-stupid-checkpoint-to-lots-of-fses-take0 fs/ext2/dir.c

fs/ext2/dir.c renamed from fs/ext2/dir.c~add-stupid-checkpoint-to-lots-of-fses-take0 +1 -0

@@ -721,4 +721,5 @@ const struct file_operations ext2_dir_op
 	.compat_ioctl	= ext2_compat_ioctl,
 #endif
 	.fsync		= ext2_sync_file,
+	.checkpoint	= generic_file_checkpoint,
 };

diff -puN fs/ext2/file.c~add-stupid-checkpoint-to-lots-of-fses-take0 fs/ext2/file.c

fs/ext2/file.c renamed from fs/ext2/file.c~add-stupid-checkpoint-to-lots-of-fses-take0 +2 -0

@@ -58,6 +58,7 @@ const struct file_operations ext2_file_o
 	.fsync		= ext2_sync_file,
 	.splice_read	= generic_file_splice_read,
 	.splice_write	= generic_file_splice_write,
+	.checkpoint	= generic_file_checkpoint,
 };
 
 #ifdef CONFIG_EXT2_FS_XIP

@@ -73,6 +74,7 @@ const struct file_operations ext2_xip_fi
 	.open		= generic_file_open,
 	.release	= ext2_release_file,
 	.fsync		= ext2_sync_file,
+	.checkpoint	= generic_file_checkpoint,
 };
 #endif
 

diff -puN fs/ext3/dir.c~add-stupid-checkpoint-to-lots-of-fses-take0 fs/ext3/dir.c

fs/ext3/dir.c renamed from fs/ext3/dir.c~add-stupid-checkpoint-to-lots-of-fses-take0 +1 -0

@@ -48,6 +48,7 @@ const struct file_operations ext3_dir_op
 #endif
 	.fsync		= ext3_sync_file,	/* BKL held */
 	.release	= ext3_release_dir,
+	.checkpoint	= generic_file_checkpoint,
 };
 
 

diff -puN fs/ext3/file.c~add-stupid-checkpoint-to-lots-of-fses-take0 fs/ext3/file.c

fs/ext3/file.c renamed from fs/ext3/file.c~add-stupid-checkpoint-to-lots-of-fses-take0 +1 -0

@@ -122,6 +122,7 @@ const struct file_operations ext3_file_o
 	.fsync		= ext3_sync_file,
 	.splice_read	= generic_file_splice_read,
 	.splice_write	= generic_file_splice_write,
+	.checkpoint	= generic_file_checkpoint,
 };
 
 const struct inode_operations ext3_file_inode_operations = {

diff -puN fs/ext4/dir.c~add-stupid-checkpoint-to-lots-of-fses-take0 fs/ext4/dir.c

fs/ext4/dir.c renamed from fs/ext4/dir.c~add-stupid-checkpoint-to-lots-of-fses-take0 +1 -0

@@ -48,6 +48,7 @@ const struct file_operations ext4_dir_op
 #endif
 	.fsync		= ext4_sync_file,
 	.release	= ext4_release_dir,
+	.checkpoint	= generic_file_checkpoint,
 };
 
 

diff -puN fs/ext4/file.c~add-stupid-checkpoint-to-lots-of-fses-take0 fs/ext4/file.c

fs/ext4/file.c renamed from fs/ext4/file.c~add-stupid-checkpoint-to-lots-of-fses-take0 +1 -0

@@ -156,6 +156,7 @@ const struct file_operations ext4_file_o
 	.fsync		= ext4_sync_file,
 	.splice_read	= generic_file_splice_read,
 	.splice_write	= generic_file_splice_write,
+	.checkpoint	= generic_file_checkpoint,
 };
 
 const struct inode_operations ext4_file_inode_operations = {

_

Quoted 26 lines · Dave Hansen

Quoted 4 lines · Alexey Dobriyan

Quoted 3 lines · Dave Hansen

Quoted 3 lines

Imagine, unsupported file is opened between userspace checks for /proc/*/checkpointable and /proc/*/fdinfo/*/checkpointable and whatever, you stil have to do all the checks inside checkpoint(2).

Quoted 8 lines

Quoted 6 lines

Alexey, we have two problems here. I completely agree that we have to do complete and thorough checks of each file descriptor at sys_checkpoint(). Any checks made at other times should not be trusted.

The other side is what Ingo has been asking for. How do we *know* when we are checkpointable *before* we call (and without calling)

This "without calling checkpoint(2)" results in much complications as demonstrated.

I'll let you take that up with Ingo. :)

Quoted 2 lines

task_struct and file are not like other structures because they are exposed in /proc.

Very true. But, we can always use the task as a proxy to say whether any of this tasks's *resources* are uncheckpointable. Is this task's ipc_namespace checkpointable, etc...

Quoted 1 line

For PROC_FS=n kernels, one can't even check.

Definitely. I'd be happy to make this check require PROC=y or even DEBUGFS=y. I just want to make the mechanism usable for developers so they're more motivated to find and fix checkpoint issues.

Quoted 6 lines

Quoted 2 lines

You can do checkpoint(2) without actual dump. You pass, you're most certainly checkpointable (with inevitable race condition in mind).

OK, so you envision this as maybe calling sys_checkpoint() with a -1 fd or something? I'm generally OK with that. If the /proc stuff is really the sticking point here, I'd be happy to stick it at the end of the series so we can throw it away more easily.

Yeah thing is I definately like what Alexey is suggesting.

The only reason for going the route of Dave's patches is to implement the pain Ingo wants to inflict to push us to faster support the resources which users actually want/need. As Alexey says that's a temporary gain and therefore not worth permanent code.

Oh, right, there's the second reason:

Quoted 5 lines

Quoted 3 lines

With time the amount of stuff C/R won't support will approach zero, but the infrastructure for "checkpointable" will stay constant. If it's too much right now, it will be way too much in future.

What have you seen in OpenVZ? Do new things that are not checkpointable pop up very often?

Realistically, do you think the uncheckpointable stuff would catch a brand-new unsupported feature? If it has a file interface then I suppose it would. Well, might. I wouldn't be surprised if the authors would cut and paste enough code to paste the .checkpoint = generic_file_checkpoint line :)

-serge
_______________________________________________
Containers mailing list Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
https://lists.linux-foundation.org/mailman/listinfo/containers

Quoted 26 lines · Dave Hansen

Quoted 4 lines · Alexey Dobriyan

Quoted 3 lines · Dave Hansen

Quoted 3 lines

Imagine, unsupported file is opened between userspace checks for /proc/*/checkpointable and /proc/*/fdinfo/*/checkpointable and whatever, you stil have to do all the checks inside checkpoint(2).

Quoted 8 lines

Quoted 6 lines

Alexey, we have two problems here. I completely agree that we have to do complete and thorough checks of each file descriptor at sys_checkpoint(). Any checks made at other times should not be trusted.

The other side is what Ingo has been asking for. How do we *know* when we are checkpointable *before* we call (and without calling)

This "without calling checkpoint(2)" results in much complications as demonstrated.

I'll let you take that up with Ingo. :)

Quoted 2 lines

task_struct and file are not like other structures because they are exposed in /proc.

Very true. But, we can always use the task as a proxy to say whether any of this tasks's *resources* are uncheckpointable. Is this task's ipc_namespace checkpointable, etc...

Quoted 1 line

For PROC_FS=n kernels, one can't even check.

Definitely. I'd be happy to make this check require PROC=y or even DEBUGFS=y. I just want to make the mechanism usable for developers so they're more motivated to find and fix checkpoint issues.

Quoted 6 lines

Quoted 2 lines

You can do checkpoint(2) without actual dump. You pass, you're most certainly checkpointable (with inevitable race condition in mind).

OK, so you envision this as maybe calling sys_checkpoint() with a -1 fd or something? I'm generally OK with that. If the /proc stuff is really the sticking point here, I'd be happy to stick it at the end of the series so we can throw it away more easily.

Yeah thing is I definately like what Alexey is suggesting.

The only reason for going the route of Dave's patches is to implement the pain Ingo wants to inflict to push us to faster support the resources which users actually want/need. As Alexey says that's a temporary gain and therefore not worth permanent code.

Oh, right, there's the second reason:

Quoted 5 lines

Quoted 3 lines

With time the amount of stuff C/R won't support will approach zero, but the infrastructure for "checkpointable" will stay constant. If it's too much right now, it will be way too much in future.

What have you seen in OpenVZ? Do new things that are not checkpointable pop up very often?

Realistically, do you think the uncheckpointable stuff would catch a brand-new unsupported feature? If it has a file interface then I suppose it would. Well, might. I wouldn't be surprised if the authors would cut and paste enough code to paste the .checkpoint = generic_file_checkpoint line :)

-serge
_______________________________________________
Containers mailing list
[email protected]
https://lists.linux-foundation.org/mailman/listinfo/containers

Quoted 11 lines · Cedric Le Goater

Quoted 9 lines

Quoted 1 line

And if Ingo's requirement is fulfilled, would any C/R patchset be acceptable ?

Yup, no matter how hideous :) Ok not really.

But the point was that it wasn't Dave not understanding Alexey's suggestion, but Greg not understanding Ingo's. If you think Ingo's goal isn't worthwhile or achievable, then argue that (as I am), don't keep elaborating on something we all agree will be needed (Alexey's suggestion or some other way of doing a true may-be-checkpointed test).

I rather spend my time on enabling things rather than forbid them.

That sure sounds productive. How could I argue with that.

But wait, haven't several teams been doing that for years? So why is c/r not in the upstream kernel? Could it be that ignoring the upstream maintainers' concerns about (a) treating the feature as a toy, (b) long-term maintainability, and (c) c/r becoming an impediment to future features, and instead hacking away at our toy feature, is *not* always the best course?

Quoted 13 lines · Serge E. Hallyn

Quoted 11 lines · Cedric Le Goater

Quoted 9 lines

Quoted 1 line

And if Ingo's requirement is fulfilled, would any C/R patchset be acceptable ?

Yup, no matter how hideous :) Ok not really.

But the point was that it wasn't Dave not understanding Alexey's suggestion, but Greg not understanding Ingo's. If you think Ingo's goal isn't worthwhile or achievable, then argue that (as I am), don't keep elaborating on something we all agree will be needed (Alexey's suggestion or some other way of doing a true may-be-checkpointed test).

I rather spend my time on enabling things rather than forbid them.

Quoted 8 lines

That sure sounds productive. How could I argue with that.

But wait, haven't several teams been doing that for years? So why is c/r not in the upstream kernel? Could it be that ignoring the upstream maintainers' concerns about (a) treating the feature as a toy, (b) long-term maintainability, and (c) c/r becoming an impediment to future features, and instead hacking away at our toy feature, is *not* always the best course?

I've been thinking about how we could make checkpoint/restart (c/r) more maintainable in the long-term. I've only come up with two ideas:

I. Implement sparse-like __cr struct annotations for some compile-time checking.

First we annotate structures which c/r needs to save. For example we might have:

	struct mm_struct {
		__cr struct vm_area_struct * mmap;
		struct rb_root mm_rb;
		struct vm_area_struct *mmap_cache;
		...
		__cr unsigned long mmap_base;
		__cr unsigned long task_size;
		..
	};

The __cr annotations indicate fields of the mm_struct which must be saved during checkpoint restart. In fact, for non-pointer fields these annotations would be sufficient to generate c/r code.

Next we would need a __cr_root annotation. These mark structures which the c/r code visits that determine the scope of c/r. If there is no path from a __cr annotation to a __cr_root annotation then we would conclude that c/r of this struct is broken. These path constraint checks could be done at compile time.