mm: move common segment checks to separate helper function (v6)
Subsystems (2)
PATCH 1/2 mm: move common segment checks to separate helper function (v6)
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.
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.
--- mm/filemap.c | 76 +++++++++++++++++++++++++++++---------------------------- 1 files changed, 39 insertions(+), 37 deletions(-)
mm/filemap.c +39 -37
@@ -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) +{ +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 ; +if (seg ==0 ) +return -EFAULT ; + *nr_segs = seg; + *count -= iv->iov_len;/* This segment is no good */ +break ; + } +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;
Signature · 1 line
1.5.0.1
mm/filemap.c success: · line 1171
Quoted 10 lines · Dmitriy Monakhov
mm/filemap.c +10
-0+/* +* 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)
Why isn't this static? Also I doubt that we want to inline this function.
Quoted 3 lines · Dmitriy Monakhov
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.
Quoted 23 lines
Changes:
- patch was split in two patches.
- comments added. I think now it is clearly describe things.
- makegeneric_segment_checks()inline
- patch prepared against 2.6.20-mm3How is tested:
- LTP test, and other readv/writev op tests.Signed-off-by Monakhov Dmitriy <[email protected]>--- mm/filemap.c | 76 +++++++++++++++++++++++++++++---------------------------- 1 files changed, 39 insertions(+), 37 deletions(-)mm/filemap.c +0
-0@@ -1159,6 +1159,39 @@ success:return size; }
mm/filemap.c success: · line 1171
Quoted 10 lines
mm/filemap.c +10
-0+/* +* 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.
Quoted 4 lines
mm/filemap.c +4
-0+{ +unsigned long seg; +for (seg =0 ; seg < *nr_segs; seg++) { +const struct iovec *iv = &iov[seg];
mm/filemap.c success: · line 1185
Quoted 10 lines
mm/filemap.c +10
-0+ +/* +* 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.
mm/filemap.c success: · line 1191
Quoted 6 lines
mm/filemap.c +6
-0+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?
Quoted 71 lines
mm/filemap.c +9
-37+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;Signature · 1 line
1.5.0.1
Quoted 11 lines · Nick Piggin
Quoted 3 lines · Dmitriy Monakhov
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.
Quoted 3 lines
Changes:
- patch was split in two patches.
Quoted 12 lines
mm/filemap.c success: · line 1171
Quoted 10 lines
mm/filemap.c +10
-0+/* +* 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.
Wow i've just carefully checked and found more functions with duplicating code:fs/xfs/linux-2.6/xfs_lrw.c:655 xfs_write()fs/ntfs/file.c:2339 ntfs_file_aio_write_nolock()
So i think nobody will object against exporting generic_segment_checks() and removing doplicating code.
Quoted 113 lines
This function name doesn't really imply that it returns you the
nr_segsand count, but that's not a big deal I guess.You also don't say that
nr_segsshould be initialised to the amount you which to write, while count must be initialised to zero.Quoted 4 lines
mm/filemap.c +4
-0+{ +unsigned long seg; +for (seg =0 ; seg < *nr_segs; seg++) { +const struct iovec *iv = &iov[seg];mm/filemap.c success: · line 1185
Quoted 10 lines
mm/filemap.c +10
-0+ +/* +* 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.
mm/filemap.c success: · line 1191
Quoted 6 lines
mm/filemap.c +6
-0+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?
Quoted 71 lines
mm/filemap.c +9
-37+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;Signature · 1 line
1.5.0.1-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Actually thinking about this a little more I don't think we should put this in. Instead this loop should be moved up to fs/read_write.c because these are checks that we want for all filesystems/drivers that use vectored I/O. We'll still need tiny loops to calculate the total I/O length for now in every method, but that will be gone as soon as we have our iodesc/uio/whatever structure.