lkml.org 
[lkml]   [2007]   [Mar]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 1/2] mm: move common segment checks to separate helper function (v6)
    On Mon, Mar 12, 2007 at 10:57:53AM +0300, Dmitriy Monakhov wrote:
    > I realy don't want to be annoying by sending this patcheset over and over
    > again. If anyone think this patch is realy cappy, please comment what
    > exectly is bad. Thank you.

    Doesn't seem like a bad idea.

    >
    > Changes:
    > - patch was split in two patches.
    > - comments added. I think now it is clearly describe things.
    > - make generic_segment_checks() inline
    > - patch prepared against 2.6.20-mm3
    >
    > How is tested:
    > - LTP test, and other readv/writev op tests.
    >
    > Signed-off-by: Monakhov Dmitriy <dmonakhov@openvz.org>
    > ---
    > mm/filemap.c | 76 +++++++++++++++++++++++++++++----------------------------
    > 1 files changed, 39 insertions(+), 37 deletions(-)
    >
    > diff --git a/mm/filemap.c b/mm/filemap.c
    > index 8e1849a..0aadf5f 100644
    > --- a/mm/filemap.c
    > +++ b/mm/filemap.c
    > @@ -1159,6 +1159,39 @@ success:
    > return size;
    > }
    >
    > +/*
    > + * Performs necessary checks before doing a write
    > + *
    > + * Adjust number of segments and amount of bytes to write.
    > + * Returns appropriate error code that caller should return or
    > + * zero in case that write should be allowed.
    > + */
    > +inline int generic_segment_checks(const struct iovec *iov,
    > + unsigned long *nr_segs, size_t *count,
    > + unsigned long access_flags)

    Make it static and not inline, and the compiler will work it out.

    This function name doesn't really imply that it returns you the
    nr_segs and count, but that's not a big deal I guess.

    You also don't say that nr_segs should be initialised to the amount
    you which to write, while count must be initialised to zero.

    > +{
    > + unsigned long seg;
    > + for (seg = 0; seg < *nr_segs; seg++) {
    > + const struct iovec *iv = &iov[seg];
    > +
    > + /*
    > + * If any segment has a negative length, or the cumulative
    > + * length ever wraps negative then return -EINVAL.
    > + */
    > + *count += iv->iov_len;
    > + if (unlikely((ssize_t)(*count|iv->iov_len) < 0))
    > + return -EINVAL;
    > + if (access_ok(access_flags, iv->iov_base, iv->iov_len))
    > + continue;

    Why now insert the above test, and put the below statements inside the
    branch? OTOH, that makes it less obviously c&p from the others. Maybe
    a subsequent patch.

    > + if (seg == 0)
    > + return -EFAULT;
    > + *nr_segs = seg;
    > + *count -= iv->iov_len; /* This segment is no good */
    > + break;
    > + }


    You could assign to *count here, once, and remove the requirement
    that the caller initialised it to zero?

    > + return 0;
    > +}
    > +
    > /**
    > * generic_file_aio_read - generic filesystem read routine
    > * @iocb: kernel I/O control block
    > @@ -1180,24 +1213,9 @@ generic_file_aio_read(struct kiocb *iocb, const struct iovec *iov,
    > loff_t *ppos = &iocb->ki_pos;
    >
    > count = 0;
    > - for (seg = 0; seg < nr_segs; seg++) {
    > - const struct iovec *iv = &iov[seg];
    > -
    > - /*
    > - * If any segment has a negative length, or the cumulative
    > - * length ever wraps negative then return -EINVAL.
    > - */
    > - count += iv->iov_len;
    > - if (unlikely((ssize_t)(count|iv->iov_len) < 0))
    > - return -EINVAL;
    > - if (access_ok(VERIFY_WRITE, iv->iov_base, iv->iov_len))
    > - continue;
    > - if (seg == 0)
    > - return -EFAULT;
    > - nr_segs = seg;
    > - count -= iv->iov_len; /* This segment is no good */
    > - break;
    > - }
    > + retval = generic_segment_checks(iov, &nr_segs, &count, VERIFY_WRITE);
    > + if (retval)
    > + return retval;
    >
    > /* coalesce the iovecs and go direct-to-BIO for O_DIRECT */
    > if (filp->f_flags & O_DIRECT) {
    > @@ -2094,30 +2112,14 @@ __generic_file_aio_write_nolock(struct kiocb *iocb, const struct iovec *iov,
    > size_t ocount; /* original count */
    > size_t count; /* after file limit checks */
    > struct inode *inode = mapping->host;
    > - unsigned long seg;
    > loff_t pos;
    > ssize_t written;
    > ssize_t err;
    >
    > ocount = 0;
    > - for (seg = 0; seg < nr_segs; seg++) {
    > - const struct iovec *iv = &iov[seg];
    > -
    > - /*
    > - * If any segment has a negative length, or the cumulative
    > - * length ever wraps negative then return -EINVAL.
    > - */
    > - ocount += iv->iov_len;
    > - if (unlikely((ssize_t)(ocount|iv->iov_len) < 0))
    > - return -EINVAL;
    > - if (access_ok(VERIFY_READ, iv->iov_base, iv->iov_len))
    > - continue;
    > - if (seg == 0)
    > - return -EFAULT;
    > - nr_segs = seg;
    > - ocount -= iv->iov_len; /* This segment is no good */
    > - break;
    > - }
    > + err = generic_segment_checks(iov, &nr_segs, &ocount, VERIFY_READ);
    > + if (err)
    > + return err;
    >
    > count = ocount;
    > pos = *ppos;
    > --
    > 1.5.0.1
    >
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2007-03-12 09:33    [W:0.048 / U:91.596 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site