On Tue, 30 Jul 2013, Azat Khuzhin wrote:
> Otherwize if there is no left space on shmem device, there will be
> "Bus error" when application will try to write to address space that was
> returned by mmap(2)
>
> This patch also preserve old behaviour if MAP_NORESERVE/VM_NORESERVE
> isset.
>
> So, with this patch, you will get next:
>
> a)
> $ echo 2 >| /proc/sys/vm/overcommit_memory
> ....
> mmap() = MAP_FAILED;
> ....
>
> b)
> ....
> mmap(0, length, PROT_READ | PROT_WRITE, MAP_SHARED | MAP_NORESERVE) =
> !MAP_FAILED;
> write()
> killed by SIGBUS
> ....
>
> c)
> $ echo 0 >| /proc/sys/vm/overcommit_memory
> ....
> mmap() = !MAP_FAILED;
> write()
> killed by SIGBUS
> ....
>
> Signed-off-by: Azat Khuzhin <a3at.m...@gmail.com>
Thanks for making the patch, but I'm afraid there are a number of
things wrong with it; and even if it were perfect, I would still be
reluctant to change the semantics of shmem_mmap() after all this time.
Some comments on your implementation below; but if getting SIGBUS from
a write to an mmapping, once the underlying filesystem (shmem/tmpfs or
any other) fills up, if that SIGBUS is troublesome for you, then please
try using fallocate() to allocate the space before accessing the mmapping.
> ---
> mm/shmem.c | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/mm/shmem.c b/mm/shmem.c
> index a87990c..965f4ba 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -32,6 +32,8 @@
> #include <linux/export.h>
> #include <linux/swap.h>
> #include <linux/aio.h>
> +#include <linux/statfs.h>
> +#include <linux/path.h>
I'm surprised you need either of those: vfs.h should have already
included statfs.h, and I don't see what path.h would be for.
>
> static struct vfsmount *shm_mnt;
>
> @@ -1356,6 +1358,20 @@ out_nomem:
>
> static int shmem_mmap(struct file *file, struct vm_area_struct *vma)
> {
> + if (!(vma->vm_flags & VM_NORESERVE) &&
> + sysctl_overcommit_memory == OVERCOMMIT_NEVER) {
So, this would be a new and different usage of sysctl_overcommit_memory:
usually it applies to vm_committed_as accounting, but you're extending
it to affect tmpfs filesystem size accounting. Hmm.
> + struct inode *inode = file_inode(file);
> + struct kstatfs sbuf;
> + u64 size;
> +
> + inode->i_sb->s_op->statfs(file->f_dentry, &sbuf);
You don't really need to go through ->statfs(), since that will arrive
at shmem_statfs(). Where you can see there will be a problem in the
case of an unlimited (max_blocks=0) mount - you will fail mmap() of
every file of non-0 size - and mmaps of 0-size files aren't much use!
But moving on from that case...
> + size = sbuf.f_bfree * sbuf.f_bsize;
> +
> + if (size < inode->i_size) {
> + return -ENOMEM;
So, if your filesystem is full, mmap() of any (i_size>0) file in it will
fail? I don't think that's what you want at all. You seem to be assuming
that no pages of the file you're mmap()ing have been allocated yet: that
may be the case, but it's very often not so.
> + }
And if we pass that test, there's stll no assurance that you won't get
SIGBUS from accessing the mmapping: nothing has actually been reserved
here, and other activity on the system can gobble up all the remaining
space in the filesystem, or take vm_committed_as to its maximum.
> + }
> +
> file_accessed(file);
> vma->vm_ops = &shmem_vm_ops;
> return 0;
> --
> 1.7.10.4
Please "man 2 fallocate" and use that instead.
Hugh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
