[PATCH nilfs-utils 0/2] trivial improvements of cleanerd

[PATCH nilfs-utils 0/2] trivial improvements of cleanerd

[Hitoshi Mitake]

This patchset removes trivial problems in cleanerd. Below is a summary of the
problems:
1. current daemonize() of cleanerd doesn't ensure not to being a session leader
2. cleanerd doesn't adjust the OOM killer

Thanks,
Hitoshi

Hitoshi Mitake (2):
  cleanerd: call _exit(2) twice for ensuring not being a session leader
  cleanerd: adjust the OOM killer

 sbin/cleanerd/cleanerd.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

-- 
1.8.1.2

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH nilfs-utils 1/2] cleanerd: call _exit(2) twice for ensuring not being a session leader

[Hitoshi Mitake]

Current daemonize() function of cleanerd call _exit(2) only once during its
process of becoming a daemon process. But in the linux environment, a daemon
process should call _exit(2) twice for ensuring not being a session leader. If a
process don't do that, unexpected SIGHUP can be sent to the process (though it
happens rarely). The signal would be confusing event for cleanerd of nilfs. This
patch removes this potential problem.

Signed-off-by: Hitoshi Mitake <mitake.hitoshi-Zyj7fXuS5i5L9jVzuh4AOg@public.gmane.org>
---
 sbin/cleanerd/cleanerd.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/sbin/cleanerd/cleanerd.c b/sbin/cleanerd/cleanerd.c
index 26067bd..edfa083 100644
--- a/sbin/cleanerd/cleanerd.c
+++ b/sbin/cleanerd/cleanerd.c
@@ -676,6 +676,16 @@ static int daemonize(int nochdir, int noclose, int nofork)
 
 	/* umask(0); */
 
+	/* for ensuring I'm not a session leader */
+	if (!nofork) {
+		pid = fork();
+		if (pid < 0)
+			return -1;
+		else if (pid != 0)
+			/* parent */
+			_exit(0);
+	}
+
 	if (!nochdir && (chdir(ROOTDIR) < 0))
 		return -1;
 
-- 
1.8.1.2

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH nilfs-utils 2/2] cleanerd: adjust the OOM killer

[Hitoshi Mitake]

cleanerd is a very important process in a system which uses nilfs. So it should
adjust the OOM killer for reducing possibility of the killing as much as
possible.

Signed-off-by: Hitoshi Mitake <mitake.hitoshi-Zyj7fXuS5i5L9jVzuh4AOg@public.gmane.org>
---
 sbin/cleanerd/cleanerd.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/sbin/cleanerd/cleanerd.c b/sbin/cleanerd/cleanerd.c
index edfa083..3494a9a 100644
--- a/sbin/cleanerd/cleanerd.c
+++ b/sbin/cleanerd/cleanerd.c
@@ -654,6 +654,40 @@ nilfs_cleanerd_select_segments(struct nilfs_cleanerd *cleanerd,
 	return nssegs;
 }
 
+static int oom_adjust(void)
+{
+	int fd, err;
+	const char *path, *score;
+	struct stat st;
+
+	/* Avoid oom-killer */
+	path = "/proc/self/oom_score_adj";
+	score = "-1000\n";
+
+	if (stat(path, &st)) {
+		/* oom_score_adj cannot be used, try oom_adj */
+		path = "/proc/self/oom_adj";
+		score = "-17\n";
+	}
+
+	fd = open(path, O_WRONLY);
+	if (fd < 0) {
+		fprintf(stderr, "can't adjust oom-killer's pardon %s, %m\n",
+			path);
+		return errno;
+	}
+
+	err = write(fd, score, strlen(score));
+	if (err < 0) {
+		fprintf(stderr, "can't adjust oom-killer's pardon %s, %m\n",
+			path);
+		close(fd);
+		return errno;
+	}
+	close(fd);
+	return 0;
+}
+
 #define DEVNULL	"/dev/null"
 #define ROOTDIR	"/"
 
@@ -1549,6 +1583,11 @@ int main(int argc, char *argv[])
 		exit(1);
 	}
 
+	if (oom_adjust() < 0) {
+		fprintf(stderr, "adjusting the OOM killer falied: %m\n");
+		exit(1);
+	}
+
 	openlog(progname, LOG_PID, LOG_DAEMON);
 	syslog(LOG_INFO, "start");
 
-- 
1.8.1.2

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH nilfs-utils 2/2] cleanerd: adjust the OOM killer

[Ryusuke Konishi]

On Wed,  1 Jan 2014 16:30:49 +0900, Hitoshi Mitake wrote:
> cleanerd is a very important process in a system which uses nilfs. So it should
> adjust the OOM killer for reducing possibility of the killing as much as
> possible.
> 
> Signed-off-by: Hitoshi Mitake <mitake.hitoshi-Zyj7fXuS5i5L9jVzuh4AOg@public.gmane.org>

Looks OK to me.

Applied after I corrected a trivial typo: s/falied/failed/.

Thanks,
Ryusuke Konishi

> ---
>  sbin/cleanerd/cleanerd.c | 39 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/sbin/cleanerd/cleanerd.c b/sbin/cleanerd/cleanerd.c
> index edfa083..3494a9a 100644
> --- a/sbin/cleanerd/cleanerd.c
> +++ b/sbin/cleanerd/cleanerd.c
> @@ -654,6 +654,40 @@ nilfs_cleanerd_select_segments(struct nilfs_cleanerd *cleanerd,
>  	return nssegs;
>  }
>  
> +static int oom_adjust(void)
> +{
> +	int fd, err;
> +	const char *path, *score;
> +	struct stat st;
> +
> +	/* Avoid oom-killer */
> +	path = "/proc/self/oom_score_adj";
> +	score = "-1000\n";
> +
> +	if (stat(path, &st)) {
> +		/* oom_score_adj cannot be used, try oom_adj */
> +		path = "/proc/self/oom_adj";
> +		score = "-17\n";
> +	}
> +
> +	fd = open(path, O_WRONLY);
> +	if (fd < 0) {
> +		fprintf(stderr, "can't adjust oom-killer's pardon %s, %m\n",
> +			path);
> +		return errno;
> +	}
> +
> +	err = write(fd, score, strlen(score));
> +	if (err < 0) {
> +		fprintf(stderr, "can't adjust oom-killer's pardon %s, %m\n",
> +			path);
> +		close(fd);
> +		return errno;
> +	}
> +	close(fd);
> +	return 0;
> +}
> +
>  #define DEVNULL	"/dev/null"
>  #define ROOTDIR	"/"
>  
> @@ -1549,6 +1583,11 @@ int main(int argc, char *argv[])
>  		exit(1);
>  	}
>  
> +	if (oom_adjust() < 0) {
> +		fprintf(stderr, "adjusting the OOM killer falied: %m\n");
> +		exit(1);
> +	}
> +
>  	openlog(progname, LOG_PID, LOG_DAEMON);
>  	syslog(LOG_INFO, "start");
>  
> -- 
> 1.8.1.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH nilfs-utils 1/2] cleanerd: call _exit(2) twice for ensuring not being a session leader

[Ryusuke Konishi]

On Wed,  1 Jan 2014 16:30:48 +0900, Hitoshi Mitake wrote:
> Current daemonize() function of cleanerd call _exit(2) only once during its
> process of becoming a daemon process. But in the linux environment, a daemon
> process should call _exit(2) twice for ensuring not being a session leader. If a
> process don't do that, unexpected SIGHUP can be sent to the process (though it
> happens rarely). The signal would be confusing event for cleanerd of nilfs. This
> patch removes this potential problem.
> 
> Signed-off-by: Hitoshi Mitake <mitake.hitoshi-Zyj7fXuS5i5L9jVzuh4AOg@public.gmane.org>
> ---
>  sbin/cleanerd/cleanerd.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/sbin/cleanerd/cleanerd.c b/sbin/cleanerd/cleanerd.c
> index 26067bd..edfa083 100644
> --- a/sbin/cleanerd/cleanerd.c
> +++ b/sbin/cleanerd/cleanerd.c
> @@ -676,6 +676,16 @@ static int daemonize(int nochdir, int noclose, int nofork)
>  
>  	/* umask(0); */
>  
> +	/* for ensuring I'm not a session leader */
> +	if (!nofork) {
> +		pid = fork();
> +		if (pid < 0)
> +			return -1;
> +		else if (pid != 0)
> +			/* parent */
> +			_exit(0);
> +	}
> +

I tried your patch, but the cleaner daemon still was a session leader.

This turned out because nilfs_cleanerd is usually executed by
mount.nilfs2 program with the nofork option (-n).

To fix this problem, it looks like the above !nofork check of the
second fork() should be removed even though it becomes confusing.  In
that case, we may need to add some explanation why fork() should be
called even if nofork is specified.


Regards,
Ryusuke Konishi


>  	if (!nochdir && (chdir(ROOTDIR) < 0))
>  		return -1;
>  
> -- 
> 1.8.1.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH nilfs-utils 1/2] cleanerd: call _exit(2) twice for ensuring not being a session leader

[Hitoshi Mitake]

On Wed, Jan 1, 2014 at 6:30 PM, Ryusuke Konishi
<konishi.ryusuke-Zyj7fXuS5i5L9jVzuh4AOg@public.gmane.org> wrote:
> On Wed,  1 Jan 2014 16:30:48 +0900, Hitoshi Mitake wrote:
>> Current daemonize() function of cleanerd call _exit(2) only once during its
>> process of becoming a daemon process. But in the linux environment, a daemon
>> process should call _exit(2) twice for ensuring not being a session leader. If a
>> process don't do that, unexpected SIGHUP can be sent to the process (though it
>> happens rarely). The signal would be confusing event for cleanerd of nilfs. This
>> patch removes this potential problem.
>>
>> Signed-off-by: Hitoshi Mitake <mitake.hitoshi-Zyj7fXuS5i5L9jVzuh4AOg@public.gmane.org>
>> ---
>>  sbin/cleanerd/cleanerd.c | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/sbin/cleanerd/cleanerd.c b/sbin/cleanerd/cleanerd.c
>> index 26067bd..edfa083 100644
>> --- a/sbin/cleanerd/cleanerd.c
>> +++ b/sbin/cleanerd/cleanerd.c
>> @@ -676,6 +676,16 @@ static int daemonize(int nochdir, int noclose, int nofork)
>>
>>       /* umask(0); */
>>
>> +     /* for ensuring I'm not a session leader */
>> +     if (!nofork) {
>> +             pid = fork();
>> +             if (pid < 0)
>> +                     return -1;
>> +             else if (pid != 0)
>> +                     /* parent */
>> +                     _exit(0);
>> +     }
>> +
>
> I tried your patch, but the cleaner daemon still was a session leader.

Thanks for your review and testing.

>
> This turned out because nilfs_cleanerd is usually executed by
> mount.nilfs2 program with the nofork option (-n).
>
> To fix this problem, it looks like the above !nofork check of the
> second fork() should be removed even though it becomes confusing.  In
> that case, we may need to add some explanation why fork() should be
> called even if nofork is specified.

For ensuring not being a session leader, fork() should be called twice. Removing
the second condition of !nofork is not enough. For this purpose, we need to
remove both of the conditions of !nofork.

BTW, what is an intention of "-n" option of cleanerd? I read the code of
nilfs_launch_cleanerd() but couldn't understand the reason of this option.

If this option is aiming to reduce calling of fork(), I think this can be
eliminated. Calling 3 fork()s (1 in mount.nilfs2, 2 in cleanerd) would be
acceptable.

Thanks,
Hitoshi
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH nilfs-utils 1/2] cleanerd: call _exit(2) twice for ensuring not being a session leader

[Ryusuke Konishi]

Hi,
On Sat, 4 Jan 2014 22:18:00 +0900, Hitoshi Mitake wrote:
> On Wed, Jan 1, 2014 at 6:30 PM, Ryusuke Konishi
> <konishi.ryusuke-Zyj7fXuS5i5L9jVzuh4AOg@public.gmane.org> wrote:
>> On Wed,  1 Jan 2014 16:30:48 +0900, Hitoshi Mitake wrote:
>>> Current daemonize() function of cleanerd call _exit(2) only once during its
>>> process of becoming a daemon process. But in the linux environment, a daemon
>>> process should call _exit(2) twice for ensuring not being a session leader. If a
>>> process don't do that, unexpected SIGHUP can be sent to the process (though it
>>> happens rarely). The signal would be confusing event for cleanerd of nilfs. This
>>> patch removes this potential problem.
>>>
>>> Signed-off-by: Hitoshi Mitake <mitake.hitoshi-Zyj7fXuS5i5L9jVzuh4AOg@public.gmane.org>
>>> ---
>>>  sbin/cleanerd/cleanerd.c | 10 ++++++++++
>>>  1 file changed, 10 insertions(+)
>>>
>>> diff --git a/sbin/cleanerd/cleanerd.c b/sbin/cleanerd/cleanerd.c
>>> index 26067bd..edfa083 100644
>>> --- a/sbin/cleanerd/cleanerd.c
>>> +++ b/sbin/cleanerd/cleanerd.c
>>> @@ -676,6 +676,16 @@ static int daemonize(int nochdir, int noclose, int nofork)
>>>
>>>       /* umask(0); */
>>>
>>> +     /* for ensuring I'm not a session leader */
>>> +     if (!nofork) {
>>> +             pid = fork();
>>> +             if (pid < 0)
>>> +                     return -1;
>>> +             else if (pid != 0)
>>> +                     /* parent */
>>> +                     _exit(0);
>>> +     }
>>> +
>>
>> I tried your patch, but the cleaner daemon still was a session leader.
> 
> Thanks for your review and testing.
> 
>>
>> This turned out because nilfs_cleanerd is usually executed by
>> mount.nilfs2 program with the nofork option (-n).
>>
>> To fix this problem, it looks like the above !nofork check of the
>> second fork() should be removed even though it becomes confusing.  In
>> that case, we may need to add some explanation why fork() should be
>> called even if nofork is specified.
> 
> For ensuring not being a session leader, fork() should be called twice. Removing
> the second condition of !nofork is not enough. For this purpose, we need to
> remove both of the conditions of !nofork.

Yes, I supposed here that the caller (the mount helper program)
already did a fork() call when -n option is specified.

But, anyway, removing only the latter check of !nofork isn't a good
idea.  It's a hacky.

> BTW, what is an intention of "-n" option of cleanerd? I read the code of
> nilfs_launch_cleanerd() but couldn't understand the reason of this option.

This is an option just to avoid fork doubly when mount.nilfs2 already
did a fork().

> If this option is aiming to reduce calling of fork(), I think this can be
> eliminated. Calling 3 fork()s (1 in mount.nilfs2, 2 in cleanerd) would be
> acceptable.

Okay, accepting 3 forks()s seems reasonable.  So, how about changing
both programs as follow?

 1) Change cleanerd to simply ignore -n option as a historical option
    (remove the existing !nofork check).
 2) Change cleanerd always fork twice to ensure that it will not be a
    session leader.
 3) Change cleaner_exec.c not to add -n option.

Thanks,
Ryusuke Konishi
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH nilfs-utils 1/2] cleanerd: call _exit(2) twice for ensuring not being a session leader

[Hitoshi Mitake]

At Sat, 04 Jan 2014 23:28:06 +0900 (JST),
Ryusuke Konishi wrote:
> 
> Hi,
> On Sat, 4 Jan 2014 22:18:00 +0900, Hitoshi Mitake wrote:
> > On Wed, Jan 1, 2014 at 6:30 PM, Ryusuke Konishi
> > <konishi.ryusuke-Zyj7fXuS5i5L9jVzuh4AOg@public.gmane.org> wrote:
> >> On Wed,  1 Jan 2014 16:30:48 +0900, Hitoshi Mitake wrote:
> >>> Current daemonize() function of cleanerd call _exit(2) only once during its
> >>> process of becoming a daemon process. But in the linux environment, a daemon
> >>> process should call _exit(2) twice for ensuring not being a session leader. If a
> >>> process don't do that, unexpected SIGHUP can be sent to the process (though it
> >>> happens rarely). The signal would be confusing event for cleanerd of nilfs. This
> >>> patch removes this potential problem.
> >>>
> >>> Signed-off-by: Hitoshi Mitake <mitake.hitoshi-Zyj7fXuS5i5L9jVzuh4AOg@public.gmane.org>
> >>> ---
> >>>  sbin/cleanerd/cleanerd.c | 10 ++++++++++
> >>>  1 file changed, 10 insertions(+)
> >>>
> >>> diff --git a/sbin/cleanerd/cleanerd.c b/sbin/cleanerd/cleanerd.c
> >>> index 26067bd..edfa083 100644
> >>> --- a/sbin/cleanerd/cleanerd.c
> >>> +++ b/sbin/cleanerd/cleanerd.c
> >>> @@ -676,6 +676,16 @@ static int daemonize(int nochdir, int noclose, int nofork)
> >>>
> >>>       /* umask(0); */
> >>>
> >>> +     /* for ensuring I'm not a session leader */
> >>> +     if (!nofork) {
> >>> +             pid = fork();
> >>> +             if (pid < 0)
> >>> +                     return -1;
> >>> +             else if (pid != 0)
> >>> +                     /* parent */
> >>> +                     _exit(0);
> >>> +     }
> >>> +
> >>
> >> I tried your patch, but the cleaner daemon still was a session leader.
> > 
> > Thanks for your review and testing.
> > 
> >>
> >> This turned out because nilfs_cleanerd is usually executed by
> >> mount.nilfs2 program with the nofork option (-n).
> >>
> >> To fix this problem, it looks like the above !nofork check of the
> >> second fork() should be removed even though it becomes confusing.  In
> >> that case, we may need to add some explanation why fork() should be
> >> called even if nofork is specified.
> > 
> > For ensuring not being a session leader, fork() should be called twice. Removing
> > the second condition of !nofork is not enough. For this purpose, we need to
> > remove both of the conditions of !nofork.
> 
> Yes, I supposed here that the caller (the mount helper program)
> already did a fork() call when -n option is specified.
> 
> But, anyway, removing only the latter check of !nofork isn't a good
> idea.  It's a hacky.
> 
> > BTW, what is an intention of "-n" option of cleanerd? I read the code of
> > nilfs_launch_cleanerd() but couldn't understand the reason of this option.
> 
> This is an option just to avoid fork doubly when mount.nilfs2 already
> did a fork().
> 
> > If this option is aiming to reduce calling of fork(), I think this can be
> > eliminated. Calling 3 fork()s (1 in mount.nilfs2, 2 in cleanerd) would be
> > acceptable.
> 
> Okay, accepting 3 forks()s seems reasonable.  So, how about changing
> both programs as follow?
> 
>  1) Change cleanerd to simply ignore -n option as a historical option
>     (remove the existing !nofork check).
>  2) Change cleanerd always fork twice to ensure that it will not be a
>     session leader.
>  3) Change cleaner_exec.c not to add -n option.

I agree with the above 3 policies. I'll send v2 based on them later.

Thanks,
Hitoshi
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html