Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove iov_iter_advance() for iter_write #12155

Merged
merged 1 commit into from Jun 1, 2021

Conversation

rincebrain
Copy link
Contributor

Motivation and Context

#12041

Description

Someone in #12041 credibly said they thought this problem might be a flip side to #11378. So I tried it.

How Has This Been Tested?

The xfs_io testcase in #12041 no longer reproduces with this patch.
(I tried comparing ZTS before and after the patch, but after the second test case with unmodified git master hung forever, I gave up and decided I'd let the buildbots deal with it.)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

@jonathonf
Copy link
Contributor

Testing applied against zfs-2.0.5-staging, can confirm no more "Attempted to advance past end of bvec iter" message.

@behlendorf
Copy link
Contributor

It seems like this should have gone with #11378.

Yes, yes it should have. @siebenmann's #12041 (comment) does a nice job explaining the issue and that the code is incorrectly advancing the iov_iter twice. The copy_from_iter() call performed during the write, zfs_write() -> zfs_uiocopy() -> zfs_uiomove_iter() -> copy_from_iter(), will advance the uio_iter the correct amount. This behavior is mentioned in some handy lwn documentation which describes the interfaces:

https://lwn.net/Articles/625077/

Note that these calls will "advance" the iterator through the buffer to correspond to the amount of data transferred. In other words, the iov_offset, count, nr_segs, and iov fields of the iterator will all be changed as needed. So two calls to copy_from_iter() will copy two successive areas from user space. Among other things, this means that the code owning the iterator must remember the base address for the iovec array, since the iov value in the iov_iter structure may change.

So I agree the right thing to do here is to just remove this unneeded iov advance, as was already done for the read case.

@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label May 30, 2021
Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

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

The code change itself looks good, but I'd suggest we update the commit message to make it clear that the advance isn't needed because it's handled by copy_from_iter().

jonathonf added a commit to jonathonf/pkgbuilds that referenced this pull request May 30, 2021
This includes two further approved backport PRs for the zfs-2.0.5-staging branch
(openzfs/zfs#12139, openzfs/zfs#12141)
and switches compat patches from 12009 to the merged commits.

It also includes bugfix openzfs/zfs#12155 .
It seems like this should have gone with openzfs#11378.

The additional iter advance is incorrect, as copy_from_iter() has
already done the right thing.

Closes: openzfs#12041

Suggested-by: @siebenmann
Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
@rincebrain
Copy link
Contributor Author

There; is something like that what you had in mind?

@behlendorf
Copy link
Contributor

That works for me.

jonathonf added a commit to jonathonf/pkgbuilds that referenced this pull request May 30, 2021
This includes two further approved backport PRs for the zfs-2.0.5-staging branch
(openzfs/zfs#12139, openzfs/zfs#12141)
and switches compat patches from 12009 to the merged commits.

It also includes bugfix openzfs/zfs#12155 .
@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Jun 1, 2021
@behlendorf behlendorf merged commit 3f81aba into openzfs:master Jun 1, 2021
tonyhutter pushed a commit that referenced this pull request Jun 2, 2021
The additional iter advance is incorrect, as copy_from_iter() has
already done the right thing.  This will result in the following
warning being printed to the console as of the 5.12 kernel.

    Attempted to advance past end of bvec iter

This change should have been included with #11378 when a
similar change was made on the read side.

Suggested-by: @siebenmann
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
Issue #11378
Closes #12041
Closes #12155
(cherry picked from commit 3f81aba)
Signed-off-by: Jonathon Fernyhough <jonathon@m2x.dev>
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Jun 2, 2021
The additional iter advance is incorrect, as copy_from_iter() has
already done the right thing.  This will result in the following
warning being printed to the console as of the 5.12 kernel.

    Attempted to advance past end of bvec iter

This change should have been included with openzfs#11378 when a
similar change was made on the read side.

Suggested-by: @siebenmann
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
Issue openzfs#11378
Closes openzfs#12041
Closes openzfs#12155
(cherry picked from commit 3f81aba)
Signed-off-by: Jonathon Fernyhough <jonathon@m2x.dev>
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Jun 3, 2021
The additional iter advance is incorrect, as copy_from_iter() has
already done the right thing.  This will result in the following
warning being printed to the console as of the 5.12 kernel.

    Attempted to advance past end of bvec iter

This change should have been included with openzfs#11378 when a
similar change was made on the read side.

Suggested-by: @siebenmann
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
Issue openzfs#11378
Closes openzfs#12041
Closes openzfs#12155
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Jun 8, 2021
The additional iter advance is incorrect, as copy_from_iter() has
already done the right thing.  This will result in the following
warning being printed to the console as of the 5.12 kernel.

    Attempted to advance past end of bvec iter

This change should have been included with openzfs#11378 when a
similar change was made on the read side.

Suggested-by: @siebenmann
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
Issue openzfs#11378
Closes openzfs#12041
Closes openzfs#12155
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Jun 9, 2021
The additional iter advance is incorrect, as copy_from_iter() has
already done the right thing.  This will result in the following
warning being printed to the console as of the 5.12 kernel.

    Attempted to advance past end of bvec iter

This change should have been included with openzfs#11378 when a
similar change was made on the read side.

Suggested-by: @siebenmann
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
Issue openzfs#11378
Closes openzfs#12041
Closes openzfs#12155
tonyhutter pushed a commit that referenced this pull request Jun 23, 2021
The additional iter advance is incorrect, as copy_from_iter() has
already done the right thing.  This will result in the following
warning being printed to the console as of the 5.12 kernel.

    Attempted to advance past end of bvec iter

This change should have been included with #11378 when a
similar change was made on the read side.

Suggested-by: @siebenmann
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Rich Ercolani <rincebrain@gmail.com>
Issue #11378
Closes #12041
Closes #12155
(cherry picked from commit 3f81aba)
Signed-off-by: Jonathon Fernyhough <jonathon@m2x.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants