From: Miklos Szeredi on
On Thu, 03 Jun 2010, Jens Axboe wrote:
> On 2010-06-03 13:32, Miklos Szeredi wrote:
> > On Thu, 03 Jun 2010, Jens Axboe wrote:
> >> I looked at that when I did the original pipe-max-pages, but it seems
> >> we're not consistent in this regard. Anyway, here's what I came up with.
> >> Not tested yet, and I hope thunderbird doesn't fsck up the patch.
> >
> > Unfortunately it did (line wrap damage).
>
> I suspected it might. This better?

Still strips spaces at the end of lines.

But bigger problem is, I don't know what this patch is against.
Doesn't apply cleanly against latest git.

Thanks,
Miklos
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Jens Axboe on
On 2010-06-03 14:45, Miklos Szeredi wrote:
> On Thu, 03 Jun 2010, Jens Axboe wrote:
>> On 2010-06-03 13:32, Miklos Szeredi wrote:
>>> On Thu, 03 Jun 2010, Jens Axboe wrote:
>>>> I looked at that when I did the original pipe-max-pages, but it seems
>>>> we're not consistent in this regard. Anyway, here's what I came up with.
>>>> Not tested yet, and I hope thunderbird doesn't fsck up the patch.
>>>
>>> Unfortunately it did (line wrap damage).
>>
>> I suspected it might. This better?
>
> Still strips spaces at the end of lines.

:-/

> But bigger problem is, I don't know what this patch is against.
> Doesn't apply cleanly against latest git.

It's not against mainline, it's against the other patches queued up in
this area.

--
Jens Axboe

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Miklos Szeredi on
On Thu, 03 Jun 2010, Jens Axboe wrote:
> On 2010-06-03 13:32, Miklos Szeredi wrote:
> > On Thu, 03 Jun 2010, Jens Axboe wrote:
> >> I looked at that when I did the original pipe-max-pages, but it seems
> >> we're not consistent in this regard. Anyway, here's what I came up with.
> >> Not tested yet, and I hope thunderbird doesn't fsck up the patch.
> >
> > Unfortunately it did (line wrap damage).
>
> I suspected it might. This better?
>
> diff --git a/fs/pipe.c b/fs/pipe.c
> index f98fae3..f7f62f3 100644
> --- a/fs/pipe.c
> +++ b/fs/pipe.c
> @@ -26,9 +26,14 @@
>
> /*
> * The max size that a non-root user is allowed to grow the pipe. Can
> - * be set by root in /proc/sys/fs/pipe-max-pages
> + * be set by root in /proc/sys/fs/pipe_max_size
> */
> -unsigned int pipe_max_pages = PIPE_DEF_BUFFERS * 16;
> +unsigned int pipe_max_size = 1048576;
> +
> +/*
> + * Minimum pipe size, as required by POSIX
> + */
> +unsigned int pipe_min_size = PAGE_SIZE;
>
> /*
> * We use a start+len construction, which provides full use of the
> @@ -1156,6 +1161,35 @@ static long pipe_set_size(struct pipe_inode_info *pipe, unsigned long nr_pages)
> return nr_pages * PAGE_SIZE;
> }
>
> +/*
> + * Currently we rely on the pipe array holding a power-of-2 number
> + * of pages.
> + */
> +static inline unsigned int round_pipe_size(unsigned int size)
> +{
> + unsigned long nr_pages;
> +
> + nr_pages = (size + PAGE_SIZE - 1) >> PAGE_SHIFT;
> + return roundup_pow_of_two(nr_pages) << PAGE_SHIFT;
> +}
> +
> +/*
> + * This should work even if CONFIG_PROC_FS isn't set, as proc_dointvec_minmax
> + * will return an error.
> + */
> +int pipe_proc_fn(struct ctl_table *table, int write, void __user *buf,
> + size_t *lenp, loff_t *ppos)
> +{
> + int ret;
> +
> + ret = proc_dointvec_minmax(table, write, buf, lenp, ppos);
> + if (ret < 0 || !write)
> + return ret;
> +
> + pipe_max_size = round_pipe_size(pipe_max_size);
> + return ret;
> +}
> +
> long pipe_fcntl(struct file *file, unsigned int cmd, unsigned long arg)
> {
> struct pipe_inode_info *pipe;
> @@ -1169,23 +1203,19 @@ long pipe_fcntl(struct file *file, unsigned int cmd, unsigned long arg)
>
> switch (cmd) {
> case F_SETPIPE_SZ: {
> - unsigned long nr_pages;
> + unsigned int size, nr_pages;
>
> - /*
> - * Currently the array must be a power-of-2 size, so adjust
> - * upwards if needed.
> - */
> - nr_pages = (arg + PAGE_SIZE - 1) >> PAGE_SHIFT;
> - nr_pages = roundup_pow_of_two(nr_pages);
> + size = round_pipe_size(arg);
> + nr_pages = size >> PAGE_SHIFT;
>
> - if (!capable(CAP_SYS_RESOURCE) && nr_pages > pipe_max_pages) {
> + if (!capable(CAP_SYS_RESOURCE) && size > pipe_max_size) {
> ret = -EPERM;
> goto out;
> - } else if (nr_pages < 1) {
> + } else if (nr_pages < PAGE_SIZE) {

This should either be "nr_pages < 1" or "size < PAGE_SIZE".

Miklos


> ret = -EINVAL;
> goto out;
> }
> - ret = pipe_set_size(pipe, arg);
> + ret = pipe_set_size(pipe, nr_pages);
> break;
> }
> case F_GETPIPE_SZ:
> diff --git a/include/linux/pipe_fs_i.h b/include/linux/pipe_fs_i.h
> index 16de393..4457969 100644
> --- a/include/linux/pipe_fs_i.h
> +++ b/include/linux/pipe_fs_i.h
> @@ -139,7 +139,9 @@ void pipe_lock(struct pipe_inode_info *);
> void pipe_unlock(struct pipe_inode_info *);
> void pipe_double_lock(struct pipe_inode_info *, struct pipe_inode_info *);
>
> -extern unsigned int pipe_max_pages;
> +extern unsigned int pipe_max_size, pipe_min_size;
> +int pipe_proc_fn(struct ctl_table *, int, void __user *, size_t *, loff_t *);
> +
>
> /* Drop the inode semaphore and wait for a pipe event, atomically */
> void pipe_wait(struct pipe_inode_info *pipe);
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 997080f..2f9b3a6 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -1471,12 +1471,12 @@ static struct ctl_table fs_table[] = {
> },
> #endif
> {
> - .procname = "pipe-max-pages",
> - .data = &pipe_max_pages,
> + .procname = "pipe_max_size",
> + .data = &pipe_max_size,
> .maxlen = sizeof(int),
> .mode = 0644,
> - .proc_handler = &proc_dointvec_minmax,
> - .extra1 = &two,
> + .proc_handler = &pipe_proc_fn,
> + .extra1 = &pipe_min_size,
> },
> /*
> * NOTE: do not add new entries to this table unless you have read
>
> --
> Jens Axboe
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Michael Kerrisk on
Hello Jens,

I've tested this F_SETPIPE_SZ code as it appears in 2.6.35-rc3, and
things seem to work as advertised, if the text below is the correct
advertisement. Could you please review the following man-pages text,
and let me know if it's okay.

fcntl.2:
F_SETPIPE_SZ (long; since Linux 2.6.35)
Change the capacity of the pipe referred to by
fd to be at least arg bytes. An unprivileged
process can adjust the pipe capacity to any
value between the system page size and the limit
defined in /proc/sys/fs/pipe-size-max (see
proc(5)). Attempts to set the pipe capacity
below the page size are silently rounded up to
the page size. Attempts by an unprivileged pro-
cess to set the pipe capacity above the limit in
/proc/sys/fs/pipe-size-max yield the error
EPERM; a privileged process (CAP_SYS_RESOURCE)
can override the limit. When allocating the
buffer for the pipe, the kernel may use a capac-
ity larger than arg, if that is convenient for
the implementation. The F_GETPIPE_SZ operation
returns the actual size used. Attempting to set
the pipe capacity smaller than the amount of
buffer space currently used to store data pro-
duces the error EBUSY.

F_GETPIPE_SZ (void; since Linux 2.6.35)
Return (as the function result) the capacity of
the pipe referred to by fd.

And in proc.5:
/proc/sys/fs/pipe-max-size (since Linux 2.6.35)
The value in this file defines an upper limit
for raising the capacity of a pipe using the
fcntl(2) F_SETPIPE_SZ operation. This limit
applies only to unprivileged processes. The
default value for this file is 1,048,576. The
value assigned to this file may be rounded
upward, to reflect the value actually employed
for a convenient implementation. To determine
the rounded-up value, display the contents of
this file after assigning a value to it. The
minimum value that can be assigned to this file
is the system page size.

Cheers,

Michael
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Jens Axboe on
On 19/06/10 07.45, Michael Kerrisk wrote:
> Hello Jens,
>
> I've tested this F_SETPIPE_SZ code as it appears in 2.6.35-rc3, and
> things seem to work as advertised, if the text below is the correct
> advertisement. Could you please review the following man-pages text,
> and let me know if it's okay.
>
> fcntl.2:
> F_SETPIPE_SZ (long; since Linux 2.6.35)
> Change the capacity of the pipe referred to by
> fd to be at least arg bytes. An unprivileged
> process can adjust the pipe capacity to any
> value between the system page size and the limit
> defined in /proc/sys/fs/pipe-size-max (see
> proc(5)). Attempts to set the pipe capacity
> below the page size are silently rounded up to
> the page size. Attempts by an unprivileged pro-
> cess to set the pipe capacity above the limit in
> /proc/sys/fs/pipe-size-max yield the error
> EPERM; a privileged process (CAP_SYS_RESOURCE)
> can override the limit. When allocating the
> buffer for the pipe, the kernel may use a capac-
> ity larger than arg, if that is convenient for
> the implementation. The F_GETPIPE_SZ operation
> returns the actual size used. Attempting to set
> the pipe capacity smaller than the amount of
> buffer space currently used to store data pro-
> duces the error EBUSY.
>
> F_GETPIPE_SZ (void; since Linux 2.6.35)
> Return (as the function result) the capacity of
> the pipe referred to by fd.
>
> And in proc.5:
> /proc/sys/fs/pipe-max-size (since Linux 2.6.35)
> The value in this file defines an upper limit
> for raising the capacity of a pipe using the
> fcntl(2) F_SETPIPE_SZ operation. This limit
> applies only to unprivileged processes. The
> default value for this file is 1,048,576. The
> value assigned to this file may be rounded
> upward, to reflect the value actually employed
> for a convenient implementation. To determine
> the rounded-up value, display the contents of
> this file after assigning a value to it. The
> minimum value that can be assigned to this file
> is the system page size.

Looks good to me.

--
Jens Axboe

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/