Skip to content

Commit

Permalink
Retire .write/.read file operations
Browse files Browse the repository at this point in the history 
The .write/.read file operations callbacks can be retired since
support for .read_iter/.write_iter and .aio_read/.aio_write has
been added.  The vfs_write()/vfs_read() entry functions will
select the correct interface for the kernel.  This is desirable
because all VFS write/read operations now rely on common code.

This change also add the generic write checks to make sure that
ulimits are enforced correctly on write.

Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Chunwei Chen <david.chen@osnexus.com>
Closes #5587 
Closes #5673
  • Loading branch information
@tuxoko @behlendorf
tuxoko authored and behlendorf committed Jan 27, 2017
1 parent 986dd8a commit 933ec99
Show file tree
Hide file tree
Showing 6 changed files with 132 additions and 37 deletions.
21 changes: 21 additions & 0 deletions config/kernel-vfs-rw-iterate.m4
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,3 +25,24 @@ AC_DEFUN([ZFS_AC_KERNEL_VFS_RW_ITERATE],
AC_MSG_RESULT(no)
])
])

dnl #
dnl # Linux 4.1.x API
dnl #
AC_DEFUN([ZFS_AC_KERNEL_GENERIC_WRITE_CHECKS],
[AC_MSG_CHECKING([whether generic_write_checks() takes kiocb])
ZFS_LINUX_TRY_COMPILE([
#include <linux/fs.h>
],[
struct kiocb *iocb = NULL;
struct iov_iter *iov = NULL;
generic_write_checks(iocb, iov);
],[
AC_MSG_RESULT(yes)
AC_DEFINE(HAVE_GENERIC_WRITE_CHECKS_KIOCB, 1,
[generic_write_checks() takes kiocb])
],[
AC_MSG_RESULT(no)
])
])
1 change: 1 addition & 0 deletions config/kernel.m4
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ AC_DEFUN([ZFS_AC_CONFIG_KERNEL], [
ZFS_AC_KERNEL_LSEEK_EXECUTE
ZFS_AC_KERNEL_VFS_ITERATE
ZFS_AC_KERNEL_VFS_RW_ITERATE
ZFS_AC_KERNEL_GENERIC_WRITE_CHECKS
ZFS_AC_KERNEL_KMAP_ATOMIC_ARGS
ZFS_AC_KERNEL_FOLLOW_DOWN_ONE
ZFS_AC_KERNEL_MAKE_REQUEST_FN
Expand Down
84 changes: 50 additions & 34 deletions module/zfs/zpl_file.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -258,21 +258,6 @@ zpl_read_common(struct inode *ip, const char *buf, size_t len, loff_t *ppos,
flags, cr, 0));
}

static ssize_t
zpl_read(struct file *filp, char __user *buf, size_t len, loff_t *ppos)
{
cred_t *cr = CRED();
ssize_t read;

crhold(cr);
read = zpl_read_common(filp->f_mapping->host, buf, len, ppos,
UIO_USERSPACE, filp->f_flags, cr);
crfree(cr);

file_accessed(filp);
return (read);
}

static ssize_t
zpl_iter_read_common(struct kiocb *kiocb, const struct iovec *iovp,
unsigned long nr_segs, size_t count, uio_seg_t seg, size_t skip)
Expand Down Expand Up @@ -311,7 +296,14 @@ static ssize_t
zpl_aio_read(struct kiocb *kiocb, const struct iovec *iovp,
unsigned long nr_segs, loff_t pos)
{
return (zpl_iter_read_common(kiocb, iovp, nr_segs, kiocb->ki_nbytes,
ssize_t ret;
size_t count;

ret = generic_segment_checks(iovp, &nr_segs, &count, VERIFY_WRITE);
if (ret)
return (ret);

return (zpl_iter_read_common(kiocb, iovp, nr_segs, count,
UIO_USERSPACE, 0));
}
#endif /* HAVE_VFS_RW_ITERATE */
Expand Down Expand Up @@ -349,6 +341,7 @@ zpl_write_common_iovec(struct inode *ip, const struct iovec *iovp, size_t count,

return (wrote);
}

inline ssize_t
zpl_write_common(struct inode *ip, const char *buf, size_t len, loff_t *ppos,
uio_seg_t segment, int flags, cred_t *cr)
Expand All @@ -362,20 +355,6 @@ zpl_write_common(struct inode *ip, const char *buf, size_t len, loff_t *ppos,
flags, cr, 0));
}

static ssize_t
zpl_write(struct file *filp, const char __user *buf, size_t len, loff_t *ppos)
{
cred_t *cr = CRED();
ssize_t wrote;

crhold(cr);
wrote = zpl_write_common(filp->f_mapping->host, buf, len, ppos,
UIO_USERSPACE, filp->f_flags, cr);
crfree(cr);

return (wrote);
}

static ssize_t
zpl_iter_write_common(struct kiocb *kiocb, const struct iovec *iovp,
unsigned long nr_segs, size_t count, uio_seg_t seg, size_t skip)
Expand All @@ -396,24 +375,63 @@ zpl_iter_write_common(struct kiocb *kiocb, const struct iovec *iovp,
static ssize_t
zpl_iter_write(struct kiocb *kiocb, struct iov_iter *from)
{
size_t count;
ssize_t ret;
uio_seg_t seg = UIO_USERSPACE;

#ifndef HAVE_GENERIC_WRITE_CHECKS_KIOCB
struct file *file = kiocb->ki_filp;
struct address_space *mapping = file->f_mapping;
struct inode *ip = mapping->host;
int isblk = S_ISBLK(ip->i_mode);

count = iov_iter_count(from);
ret = generic_write_checks(file, &kiocb->ki_pos, &count, isblk);
#else
/*
* XXX - ideally this check should be in the same lock region with
* write operations, so that there's no TOCTTOU race when doing
* append and someone else grow the file.
*/
ret = generic_write_checks(kiocb, from);
count = ret;
#endif
if (ret <= 0)
return (ret);

if (from->type & ITER_KVEC)
seg = UIO_SYSSPACE;
if (from->type & ITER_BVEC)
seg = UIO_BVEC;

ret = zpl_iter_write_common(kiocb, from->iov, from->nr_segs,
iov_iter_count(from), seg, from->iov_offset);
count, seg, from->iov_offset);
if (ret > 0)
iov_iter_advance(from, ret);

return (ret);
}
#else
static ssize_t
zpl_aio_write(struct kiocb *kiocb, const struct iovec *iovp,
unsigned long nr_segs, loff_t pos)
{
return (zpl_iter_write_common(kiocb, iovp, nr_segs, kiocb->ki_nbytes,
struct file *file = kiocb->ki_filp;
struct address_space *mapping = file->f_mapping;
struct inode *ip = mapping->host;
int isblk = S_ISBLK(ip->i_mode);
size_t count;
ssize_t ret;

ret = generic_segment_checks(iovp, &nr_segs, &count, VERIFY_READ);
if (ret)
return (ret);

ret = generic_write_checks(file, &pos, &count, isblk);
if (ret)
return (ret);

return (zpl_iter_write_common(kiocb, iovp, nr_segs, count,
UIO_USERSPACE, 0));
}
#endif /* HAVE_VFS_RW_ITERATE */
Expand Down Expand Up @@ -835,8 +853,6 @@ const struct file_operations zpl_file_operations = {
.open = zpl_open,
.release = zpl_release,
.llseek = zpl_llseek,
.read = zpl_read,
.write = zpl_write,
#ifdef HAVE_VFS_RW_ITERATE
.read_iter = zpl_iter_read,
.write_iter = zpl_iter_write,
Expand Down
5 changes: 3 additions & 2 deletions tests/runfiles/linux.run
View file
Original file line number Diff line number Diff line change
Expand Up @@ -458,8 +458,9 @@ tests = ['inuse_004_pos']
post =

# DISABLED: needs investigation
#[tests/functional/large_files]
#tests = ['large_files_001_pos']
# large_files_001_pos
[tests/functional/large_files]
tests = ['large_files_002_pos']

# DISABLED: needs investigation
#[tests/functional/largest_pool]
Expand Down
3 changes: 2 additions & 1 deletion tests/zfs-tests/tests/functional/large_files/Makefile.am
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,5 @@ pkgdatadir = $(datadir)/@PACKAGE@/zfs-tests/tests/functional/large_files
dist_pkgdata_SCRIPTS = \
setup.ksh \
cleanup.ksh \
large_files_001_pos.ksh
large_files_001_pos.ksh \
large_files_002_pos.ksh
55 changes: 55 additions & 0 deletions tests/zfs-tests/tests/functional/large_files/large_files_002_pos.ksh
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
#!/bin/ksh -p
#
# CDDL HEADER START
#
# The contents of this file are subject to the terms of the
# Common Development and Distribution License (the "License").
# You may not use this file except in compliance with the License.
#
# You can obtain a copy of the license at usr/src/OPENSOLARIS.LICENSE
# or http://www.opensolaris.org/os/licensing.
# See the License for the specific language governing permissions
# and limitations under the License.
#
# When distributing Covered Code, include this CDDL HEADER in each
# file and include the License file at usr/src/OPENSOLARIS.LICENSE.
# If applicable, add the following below this CDDL HEADER, with the
# fields enclosed by brackets "[]" replaced with your own identifying
# information: Portions Copyright [yyyy] [name of copyright owner]
#
# CDDL HEADER END
#

#
# Copyright (c) 2015 by Lawrence Livermore National Security, LLC.
# All rights reserved.
#

. $STF_SUITE/include/libtest.shlib

#
# DESCRIPTION:
# Verify 'ulimit -f' file size restrictions are enforced.
#
# STRATEGY:
# 1. Set ulimit file size to unlimited, verify files can be created.
# 2. Reduce ulimit file size, verify the expected error is returned.
#

verify_runnable "both"

log_assert "Verify 'ulimit -f' maximum file size is enforced"

# Verify 'ulimit -f unlimited' works
log_must ulimit -f unlimited
log_must sh -c 'dd if=/dev/zero of=$TESTDIR/ulimit_write_file bs=1M count=2'
log_must sh -c '$TRUNCATE -s2M $TESTDIR/ulimit_trunc_file'
log_must $RM $TESTDIR/ulimit_write_file $TESTDIR/ulimit_trunc_file

# Verify 'ulimit -f <size>' works
log_must ulimit -f 1024
log_mustnot sh -c 'dd if=/dev/zero of=$TESTDIR/ulimit_write_file bs=1M count=2'
log_mustnot sh -c '$TRUNCATE -s2M $TESTDIR/ulimit_trunc_file'
log_must $RM $TESTDIR/ulimit_write_file $TESTDIR/ulimit_trunc_file

log_pass "Successfully enforced 'ulimit -f' maximum file size"

4 comments on commit 933ec99

@grizzlyfred
Copy link

Choose a reason for hiding this comment

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

I do not know if something went wrong here, but I am trying to build freshly pulled zfs up to 3b6425c "Fix atomic_sub_64() i386 assembly implementation" due to the fact that fedora 25 is on Kernel 4.9 but 0.7.x-zfs is needed. I get the following build error(s):

»zpl_iter_write«...
.../zfs/module/zfs/zpl_file.c:389:29: [-Werror=incompatible-pointer-types]
ret = generic_write_checks(file, &kiocb->ki_pos, &count, isblk);
^~~~
In file included from ./include/linux/compat.h:17:0,
from ../zfs/module/zfs/zpl_file.c:28:
./include/linux/fs.h:2794:16: »struct kiocb *« expected but arg has type »struct file *«
extern ssize_t generic_write_checks(struct kiocb *, struct iov_iter *);
^~~~~~~~~~~~~~~~~~~~
.../zfs/module/zfs/zpl_file.c:389:35: arg 2 of »generic_write_checks« [-Werror=incompatible-pointer-types]
ret = generic_write_checks(file, &kiocb->ki_pos, &count, isblk);
^
In file included from ./include/linux/compat.h:17:0,
from .../zfs/module/zfs/zpl_file.c:28:
./include/linux/fs.h:2794:16: »struct iov_iter *« expected, but arg type is »loff_t * {aka long long int *}«
extern ssize_t generic_write_checks(struct kiocb *, struct iov_iter *);
^~~~~~~~~~~~~~~~~~~~
../zfs/zpl_file.c:389:8: too many arguments for function »generic_write_checks«
ret = generic_write_checks(file, &kiocb->ki_pos, &count, isblk);
^~~~~~~~~~~~~~~~~~~

... so I am not sure if in line 382 the "ifndef" should be a ifdef to get the two different interfaces (as in "if with kiocb use 4-paramter fundtion, else the simple one?

@tuxoko
Copy link
Contributor Author

@tuxoko tuxoko commented on 933ec99 Feb 1, 2017

Choose a reason for hiding this comment

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

@grizzlyfred
Please post your config.log for zfs to #5720

@grizzlyfred
Copy link

Choose a reason for hiding this comment

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

Are you sure this is related? OK, that is beyond my understanding of the inner workings of ZoL.
Anyway, i hard-reseted to commit -1 of this one: 986dd8a ("OpenZFS 5561 - support root pools on EFI/GPT partitioned disks", 2017-01-27) and now I have at least some kmod rpms, spl I left at whre it was some hrs. ago. I am now specifically on this specific commit, to see if it introduces the build fault or not ("bisect..").
I would post the config log as close as possible to the build error.

@grizzlyfred
Copy link

@grizzlyfred grizzlyfred commented on 933ec99 Feb 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@tuxoko I can confirm this exact commit fails to build, haven't checked further though I would have liked to catch last weeks bugfixes for now. As requested, config.log is uploaded on #5720.

Update: tuxoko figured out, that the "old" trap of not running ./autogen.sh was the culprit.
Keep in mind "git pull" will NOT delete the "old" configure script (from last week...)

Please sign in to comment.