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.

Sign up for GitHub

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

SEEK_HOLE should not block on txg_wait_synced() #5962

Closed
wants to merge 2 commits into from
Closed

SEEK_HOLE should not block on txg_wait_synced() #5962

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
99b2905
SEEK_HOLE should not block on txg_wait_synced()
dbavatar Mar 24, 2017
01de657
Add zfs_dmu_offset_next_sync
dbavatar Apr 12, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions man/man5/zfs-module-parameters.5
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1427,6 +1427,20 @@ Enable NOP writes
Use \fB1\fR for yes (default) and \fB0\fR to disable.
.RE

.sp
.ne 2
.na
\fBzfs_dmu_offset_next_sync\fR (int)
.ad
.RS 12n
Enable forcing txg sync to find holes. When enabled forces ZFS to act
like prior versions when SEEK_HOLE or SEEK_DATA flags are used, which
when a dnode is dirty causes txg's to be synced so that this data can be
found.
.sp
Use \fB1\fR for yes and \fB0\fR to disable (default).
.RE

.sp
.ne 2
.na
Expand Down
49 changes: 42 additions & 7 deletions module/zfs/dmu.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,11 @@ int zfs_nopwrite_enabled = 1;
*/
unsigned long zfs_per_txg_dirty_frees_percent = 30;

/*
* Enable/disable forcing txg sync when dirty in dmu_offset_next.
*/
int zfs_dmu_offset_next_sync = 0;

const dmu_object_type_info_t dmu_ot[DMU_OT_NUMTYPES] = {
{ DMU_BSWAP_UINT8, TRUE, "unallocated" },
{ DMU_BSWAP_ZAP, TRUE, "object directory" },
Expand Down Expand Up @@ -1989,32 +1994,57 @@ dmu_write_policy(objset_t *os, dnode_t *dn, int level, int wp,
zp->zp_nopwrite = nopwrite;
}

/*
* This fn is only called from zfs_holey_common for zpl_llseek
* Previously, this required flushing of all the TXG's associated with this
* file in order to scan the dnode for holes. This caused extremely poor
* performance for dirty files, for example log files. As a compromise, if a
* dnode is clean, then we can provide hole data. However if a dnode is dirty,
* simply fall back to what we would have done if we did not support seeking
* holes in this vfsop at all, which is a valid thing to do.
*/
int
dmu_offset_next(objset_t *os, uint64_t object, boolean_t hole, uint64_t *off)
{
dnode_t *dn;
int i, err;
boolean_t clean = B_TRUE;

err = dnode_hold(os, object, FTAG, &dn);
if (err)
return (err);

/*
* Sync any current changes before
* we go trundling through the block pointers.
* Check if dnode is dirty
*/
for (i = 0; i < TXG_SIZE; i++) {
if (list_link_active(&dn->dn_dirty_link[i]))
break;
if (dn->dn_dirtyctx != DN_UNDIRTIED) {
for (i = 0; i < TXG_SIZE; i++) {
if (!list_is_empty(&dn->dn_dirty_records[i])) {
clean = B_FALSE;
break;
}
}
}
if (i != TXG_SIZE) {

/*
* If compatibility option is on, sync any current changes before
* we go trundling through the block pointers.
*/
if (!clean && zfs_dmu_offset_next_sync) {
clean = B_TRUE;
dnode_rele(dn, FTAG);
txg_wait_synced(dmu_objset_pool(os), 0);
err = dnode_hold(os, object, FTAG, &dn);
if (err)
return (err);
}

err = dnode_next_offset(dn, (hole ? DNODE_FIND_HOLE : 0), off, 1, 1, 0);
if (clean)
err = dnode_next_offset(dn,
(hole ? DNODE_FIND_HOLE : 0), off, 1, 1, 0);
else
err = SET_ERROR(EBUSY);

dnode_rele(dn, FTAG);

return (err);
Expand Down Expand Up @@ -2238,5 +2268,10 @@ MODULE_PARM_DESC(zfs_nopwrite_enabled, "Enable NOP writes");
module_param(zfs_per_txg_dirty_frees_percent, ulong, 0644);
MODULE_PARM_DESC(zfs_per_txg_dirty_frees_percent,
"percentage of dirtied blocks from frees in one TXG");

module_param(zfs_dmu_offset_next_sync, int, 0644);
MODULE_PARM_DESC(zfs_dmu_offset_next_sync, "Enable forcing txg sync to find holes");

/* END CSTYLED */

#endif
4 changes: 4 additions & 0 deletions module/zfs/zfs_vnops.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,10 @@ zfs_holey_common(struct inode *ip, int cmd, loff_t *off)
if (error == ESRCH)
return (SET_ERROR(ENXIO));

/* file was dirty, so fall back to using file_sz logic */
if (error == EBUSY)
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than suppress this error I think it would be better to return immediately and allow it to percolate up to zpl_llseek() and then handle it there.

Copy link
Contributor

@ryao ryao Apr 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

As I said in the comment thread and I am adding here for completeness, I disagree. The correct place is in zfs_vnops.c because there is no zpl_* analogue on other platforms. We'd risk introducing inconsistent behavior between platforms when the patch is adopted if someone misses the key change in the zpl_ function. Even if they do, they will need to put it here anyway and then we will need to do another patch to bring the platforms in sync.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, upon further reflection I'm OK with that and withdraw my objection. The existing patch leaves this check where it is.

error = 0;

/*
* We could find a hole that begins after the logical end-of-file,
* because dmu_offset_next() only works on whole blocks. If the
Expand Down