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

Retire .write/.read file operations #5673

Merged
merged 1 commit into from
Jan 27, 2017
Merged

Conversation

tuxoko
Copy link
Contributor

@tuxoko tuxoko commented Jan 26, 2017

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. A similar check was
added to zpl_fallocate() so the truncate case is also handled
properly.

Signed-off-by: Brian Behlendorf [email protected]
Signed-off-by: Chunwei Chen [email protected]
Issue #5587

@mention-bot
Copy link

@tuxoko, thanks for your PR! By analyzing the history of the files in this pull request, we identified @dweeezil, @dechamps and @chenhaiq to be potential reviewers.

@behlendorf
Copy link
Contributor

@tuxoko thank you for picking this one up for me. I'll give it another review.

@tuxoko
Copy link
Contributor Author

tuxoko commented Jan 27, 2017

One thing that bothers me is that the generic_write_checks should be in the same lock region as the write operations. So to ensure no time of check to use race, ideally the check needs to happen in zfs_write layer.

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.

Functionally it looks good, we'll see what the testing says. You'll want to resubmit this to get a clean testing run with a fixed runfile.

#tests = ['large_files_001_pos']
[tests/functional/large_files]
tests = ['large_files_001_pos']
tests = ['large_files_002_pos']
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be the following so both run.

[tests/functional/large_files]
tests = ['large_files_001_pos', 'large_files_002_pos']

@behlendorf
Copy link
Contributor

generic_write_checks should be in the same lock region as the write operations.

Specifically under the range lock to handle the concurrent append case? We don't pass the pos variable updated from generic_write_checks() down to zfs_write() so I believe the worst that could happen is we overrun the rlimit by a few system calls. That's not ideal, but it's probably OK in practice since it would be painful to push this check down farther. Or are you concerned about a different check?

@tuxoko
Copy link
Contributor Author

tuxoko commented Jan 27, 2017

@behlendorf Yes, that's what I was talking about.

@behlendorf
Copy link
Contributor

Adding a comment describing this potential issues for our future selves wouldn't be a terrible idea.

@tuxoko
Copy link
Contributor Author

tuxoko commented Jan 27, 2017

OK, also the check in fallocate seems to be wrong. We never grow size with fallocate anyway.

@behlendorf
Copy link
Contributor

behlendorf commented Jan 27, 2017

@tuxoko wrong or just unnecessary? Since we currently only allow hole punching I agree that case can't currently happen, but if/when we do it'll be needed.

@tuxoko
Copy link
Contributor Author

tuxoko commented Jan 27, 2017

@behlendorf
It's wrong, because we only support hole punching and hole punching implies keep size. So size is never changed and the call should not fail because of a large len.

@behlendorf
Copy link
Contributor

Yes, I see. OK, let's drop that hunk from the patch.

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 <[email protected]>
Signed-off-by: Chunwei Chen <[email protected]>
Issue openzfs#5587
@behlendorf behlendorf merged commit 933ec99 into openzfs:master Jan 27, 2017
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Feb 2, 2017
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 <[email protected]>
Signed-off-by: Chunwei Chen <[email protected]>
Closes openzfs#5587
Closes openzfs#5673
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Feb 2, 2017
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 <[email protected]>
Signed-off-by: Chunwei Chen <[email protected]>
Closes openzfs#5587
Closes openzfs#5673
Requires-builders: style
behlendorf pushed a commit that referenced this pull request Feb 3, 2017
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 <[email protected]>
Signed-off-by: Chunwei Chen <[email protected]>
Closes #5587
Closes #5673
wli5 pushed a commit to wli5/zfs that referenced this pull request Feb 28, 2017
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 <[email protected]>
Signed-off-by: Chunwei Chen <[email protected]>
Closes openzfs#5587 
Closes openzfs#5673
wli5 pushed a commit to wli5/zfs that referenced this pull request Feb 28, 2017
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 <[email protected]>
Signed-off-by: Chunwei Chen <[email protected]>
Closes openzfs#5587
Closes openzfs#5673
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants