Skip to content

Commit 63f1b92

Browse files
robnspauka
robn
authored and
spauka
committed
ZIL: allow zil_commit() to fail with error
This changes zil_commit() to have an int return, and updates all callers to check it. There are no corresponding internal changes yet; it will always return 0. Since zil_commit() is an indication that the caller _really_ wants the associated data to be durability stored, I've annotated it with the __warn_unused_result__ compiler attribute (via __must_check), to emit a warning if it's ever ussd without doing something with the return code. I hope this will mean we never misuse it in the future. Sponsored-by: Klara, Inc. Sponsored-by: Wasabi Technology, Inc. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Alexander Motin <alexander.motin@TrueNAS.com> Signed-off-by: Rob Norris <rob.norris@klarasystems.com> Closes openzfs#17398
1 parent 56deab2 commit 63f1b92

16 files changed

Lines changed: 148 additions & 86 deletions

File tree

‎cmd/ztest.c‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2965,7 +2965,7 @@ ztest_zil_commit(ztest_ds_t *zd, uint64_t id)
29652965

29662966
(void) pthread_rwlock_rdlock(&zd->zd_zilog_lock);
29672967

2968-
zil_commit(zilog, ztest_random(ZTEST_OBJECTS));
2968+
VERIFY0(zil_commit(zilog, ztest_random(ZTEST_OBJECTS)));
29692969

29702970
/*
29712971
* Remember the committed values in zd, which is in parent/child
@@ -7936,7 +7936,7 @@ ztest_freeze(void)
79367936
*/
79377937
while (BP_IS_HOLE(&zd->zd_zilog->zl_header->zh_log)) {
79387938
ztest_dmu_object_alloc_free(zd, 0);
7939-
zil_commit(zd->zd_zilog, 0);
7939+
VERIFY0(zil_commit(zd->zd_zilog, 0));
79407940
}
79417941

79427942
txg_wait_synced(spa_get_dsl(spa), 0);
@@ -7978,7 +7978,7 @@ ztest_freeze(void)
79787978
/*
79797979
* Commit all of the changes we just generated.
79807980
*/
7981-
zil_commit(zd->zd_zilog, 0);
7981+
VERIFY0(zil_commit(zd->zd_zilog, 0));
79827982
txg_wait_synced(spa_get_dsl(spa), 0);
79837983

79847984
/*

‎include/os/freebsd/spl/sys/debug.h‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,10 @@
6969
#define __maybe_unused __attribute__((unused))
7070
#endif
7171

72+
#ifndef __must_check
73+
#define __must_check __attribute__((__warn_unused_result__))
74+
#endif
75+
7276
/*
7377
* Without this, we see warnings from objtool during normal Linux builds when
7478
* the kernel is built with CONFIG_STACK_VALIDATION=y:

‎include/os/linux/spl/sys/debug.h‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,10 @@
6969
#define __maybe_unused __attribute__((unused))
7070
#endif
7171

72+
#ifndef __must_check
73+
#define __must_check __attribute__((__warn_unused_result__))
74+
#endif
75+
7276
/*
7377
* Without this, we see warnings from objtool during normal Linux builds when
7478
* the kernel is built with CONFIG_STACK_VALIDATION=y:

‎include/sys/zil.h‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -610,8 +610,7 @@ extern void zil_itx_destroy(itx_t *itx);
610610
extern void zil_itx_assign(zilog_t *zilog, itx_t *itx, dmu_tx_t *tx);
611611

612612
extern void zil_async_to_sync(zilog_t *zilog, uint64_t oid);
613-
extern void zil_commit(zilog_t *zilog, uint64_t oid);
614-
extern void zil_commit_impl(zilog_t *zilog, uint64_t oid);
613+
extern int __must_check zil_commit(zilog_t *zilog, uint64_t oid);
615614
extern void zil_remove_async(zilog_t *zilog, uint64_t oid);
616615

617616
extern int zil_reset(const char *osname, void *txarg);

‎lib/libspl/include/sys/debug.h‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,4 +38,8 @@
3838
#define __maybe_unused __attribute__((unused))
3939
#endif
4040

41+
#ifndef __must_check
42+
#define __must_check __attribute__((warn_unused_result))
43+
#endif
44+
4145
#endif

‎module/os/freebsd/zfs/zfs_vfsops.c‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -455,8 +455,13 @@ zfs_sync(vfs_t *vfsp, int waitfor)
455455
return (0);
456456
}
457457

458-
if (zfsvfs->z_log != NULL)
459-
zil_commit(zfsvfs->z_log, 0);
458+
if (zfsvfs->z_log != NULL) {
459+
error = zil_commit(zfsvfs->z_log, 0);
460+
if (error != 0) {
461+
zfs_exit(zfsvfs, FTAG);
462+
return (error);
463+
}
464+
}
460465

461466
zfs_exit(zfsvfs, FTAG);
462467
} else {

‎module/os/freebsd/zfs/zfs_vnops_os.c‎

Lines changed: 26 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -1193,8 +1193,8 @@ zfs_create(znode_t *dzp, const char *name, vattr_t *vap, int excl, int mode,
11931193
*zpp = zp;
11941194
}
11951195

1196-
if (zfsvfs->z_os->os_sync == ZFS_SYNC_ALWAYS)
1197-
zil_commit(zilog, 0);
1196+
if (error == 0 && zfsvfs->z_os->os_sync == ZFS_SYNC_ALWAYS)
1197+
error = zil_commit(zilog, 0);
11981198

11991199
zfs_exit(zfsvfs, FTAG);
12001200
return (error);
@@ -1323,9 +1323,8 @@ zfs_remove_(vnode_t *dvp, vnode_t *vp, const char *name, cred_t *cr)
13231323
if (xzp)
13241324
vrele(ZTOV(xzp));
13251325

1326-
if (zfsvfs->z_os->os_sync == ZFS_SYNC_ALWAYS)
1327-
zil_commit(zilog, 0);
1328-
1326+
if (error == 0 && zfsvfs->z_os->os_sync == ZFS_SYNC_ALWAYS)
1327+
error = zil_commit(zilog, 0);
13291328

13301329
zfs_exit(zfsvfs, FTAG);
13311330
return (error);
@@ -1556,8 +1555,8 @@ zfs_mkdir(znode_t *dzp, const char *dirname, vattr_t *vap, znode_t **zpp,
15561555

15571556
getnewvnode_drop_reserve();
15581557

1559-
if (zfsvfs->z_os->os_sync == ZFS_SYNC_ALWAYS)
1560-
zil_commit(zilog, 0);
1558+
if (error == 0 && zfsvfs->z_os->os_sync == ZFS_SYNC_ALWAYS)
1559+
error = zil_commit(zilog, 0);
15611560

15621561
zfs_exit(zfsvfs, FTAG);
15631562
return (error);
@@ -1637,8 +1636,8 @@ zfs_rmdir_(vnode_t *dvp, vnode_t *vp, const char *name, cred_t *cr)
16371636
if (zfsvfs->z_use_namecache)
16381637
cache_vop_rmdir(dvp, vp);
16391638
out:
1640-
if (zfsvfs->z_os->os_sync == ZFS_SYNC_ALWAYS)
1641-
zil_commit(zilog, 0);
1639+
if (error == 0 && zfsvfs->z_os->os_sync == ZFS_SYNC_ALWAYS)
1640+
error = zil_commit(zilog, 0);
16421641

16431642
zfs_exit(zfsvfs, FTAG);
16441643
return (error);
@@ -3009,8 +3008,8 @@ zfs_setattr(znode_t *zp, vattr_t *vap, int flags, cred_t *cr, zidmap_t *mnt_ns)
30093008
}
30103009

30113010
out2:
3012-
if (os->os_sync == ZFS_SYNC_ALWAYS)
3013-
zil_commit(zilog, 0);
3011+
if (err == 0 && os->os_sync == ZFS_SYNC_ALWAYS)
3012+
err = zil_commit(zilog, 0);
30143013

30153014
zfs_exit(zfsvfs, FTAG);
30163015
return (err);
@@ -3539,7 +3538,7 @@ zfs_do_rename_impl(vnode_t *sdvp, vnode_t **svpp, struct componentname *scnp,
35393538

35403539
out:
35413540
if (error == 0 && zfsvfs->z_os->os_sync == ZFS_SYNC_ALWAYS)
3542-
zil_commit(zilog, 0);
3541+
error = zil_commit(zilog, 0);
35433542
zfs_exit(zfsvfs, FTAG);
35443543

35453544
return (error);
@@ -3731,7 +3730,7 @@ zfs_symlink(znode_t *dzp, const char *name, vattr_t *vap,
37313730
*zpp = zp;
37323731

37333732
if (zfsvfs->z_os->os_sync == ZFS_SYNC_ALWAYS)
3734-
zil_commit(zilog, 0);
3733+
error = zil_commit(zilog, 0);
37353734
}
37363735

37373736
zfs_exit(zfsvfs, FTAG);
@@ -3921,8 +3920,8 @@ zfs_link(znode_t *tdzp, znode_t *szp, const char *name, cred_t *cr,
39213920
vnevent_link(ZTOV(szp), ct);
39223921
}
39233922

3924-
if (zfsvfs->z_os->os_sync == ZFS_SYNC_ALWAYS)
3925-
zil_commit(zilog, 0);
3923+
if (error == 0 && zfsvfs->z_os->os_sync == ZFS_SYNC_ALWAYS)
3924+
error = zil_commit(zilog, 0);
39263925

39273926
zfs_exit(zfsvfs, FTAG);
39283927
return (error);
@@ -4510,8 +4509,13 @@ zfs_putpages(struct vnode *vp, vm_page_t *ma, size_t len, int flags,
45104509

45114510
out:
45124511
zfs_rangelock_exit(lr);
4513-
if (commit)
4514-
zil_commit(zfsvfs->z_log, zp->z_id);
4512+
if (commit) {
4513+
err = zil_commit(zfsvfs->z_log, zp->z_id);
4514+
if (err != 0) {
4515+
zfs_exit(zfsvfs, FTAG);
4516+
return (err);
4517+
}
4518+
}
45154519

45164520
dataset_kstats_update_write_kstats(&zfsvfs->z_kstat, len);
45174521

@@ -6773,9 +6777,11 @@ zfs_deallocate(struct vop_deallocate_args *ap)
67736777
if (error == 0) {
67746778
if (zfsvfs->z_os->os_sync == ZFS_SYNC_ALWAYS ||
67756779
(ap->a_ioflag & IO_SYNC) != 0)
6776-
zil_commit(zilog, zp->z_id);
6777-
*ap->a_offset = off + len;
6778-
*ap->a_len = 0;
6780+
error = zil_commit(zilog, zp->z_id);
6781+
if (error == 0) {
6782+
*ap->a_offset = off + len;
6783+
*ap->a_len = 0;
6784+
}
67796785
}
67806786

67816787
zfs_exit(zfsvfs, FTAG);

‎module/os/freebsd/zfs/zvol_os.c‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -727,9 +727,9 @@ zvol_strategy_impl(zv_request_t *zvr)
727727
break;
728728
}
729729

730-
if (commit) {
730+
if (error == 0 && commit) {
731731
commit:
732-
zil_commit(zv->zv_zilog, ZVOL_OBJ);
732+
error = zil_commit(zv->zv_zilog, ZVOL_OBJ);
733733
}
734734
resume:
735735
rw_exit(&zv->zv_suspend_lock);
@@ -906,8 +906,8 @@ zvol_cdev_write(struct cdev *dev, struct uio *uio_s, int ioflag)
906906
zfs_rangelock_exit(lr);
907907
int64_t nwritten = start_resid - zfs_uio_resid(&uio);
908908
dataset_kstats_update_write_kstats(&zv->zv_kstat, nwritten);
909-
if (commit)
910-
zil_commit(zv->zv_zilog, ZVOL_OBJ);
909+
if (error == 0 && commit)
910+
error = zil_commit(zv->zv_zilog, ZVOL_OBJ);
911911
rw_exit(&zv->zv_suspend_lock);
912912

913913
return (error);
@@ -1117,7 +1117,7 @@ zvol_cdev_ioctl(struct cdev *dev, ulong_t cmd, caddr_t data,
11171117
case DIOCGFLUSH:
11181118
rw_enter(&zv->zv_suspend_lock, ZVOL_RW_READER);
11191119
if (zv->zv_zilog != NULL)
1120-
zil_commit(zv->zv_zilog, ZVOL_OBJ);
1120+
error = zil_commit(zv->zv_zilog, ZVOL_OBJ);
11211121
rw_exit(&zv->zv_suspend_lock);
11221122
break;
11231123
case DIOCGDELETE:
@@ -1152,7 +1152,7 @@ zvol_cdev_ioctl(struct cdev *dev, ulong_t cmd, caddr_t data,
11521152
}
11531153
zfs_rangelock_exit(lr);
11541154
if (sync)
1155-
zil_commit(zv->zv_zilog, ZVOL_OBJ);
1155+
error = zil_commit(zv->zv_zilog, ZVOL_OBJ);
11561156
rw_exit(&zv->zv_suspend_lock);
11571157
break;
11581158
case DIOCGSTRIPESIZE:

‎module/os/linux/zfs/zfs_vfsops.c‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -288,10 +288,10 @@ zfs_sync(struct super_block *sb, int wait, cred_t *cr)
288288
return (SET_ERROR(EIO));
289289
}
290290

291-
zil_commit(zfsvfs->z_log, 0);
291+
err = zil_commit(zfsvfs->z_log, 0);
292292
zfs_exit(zfsvfs, FTAG);
293293

294-
return (0);
294+
return (err);
295295
}
296296

297297
static void

‎module/os/linux/zfs/zfs_vnops_os.c‎

Lines changed: 36 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -841,8 +841,8 @@ zfs_create(znode_t *dzp, char *name, vattr_t *vap, int excl,
841841
*zpp = zp;
842842
}
843843

844-
if (zfsvfs->z_os->os_sync == ZFS_SYNC_ALWAYS)
845-
zil_commit(zilog, 0);
844+
if (error == 0 && zfsvfs->z_os->os_sync == ZFS_SYNC_ALWAYS)
845+
error = zil_commit(zilog, 0);
846846

847847
zfs_exit(zfsvfs, FTAG);
848848
return (error);
@@ -1203,8 +1203,8 @@ zfs_remove(znode_t *dzp, char *name, cred_t *cr, int flags)
12031203
zfs_zrele_async(xzp);
12041204
}
12051205

1206-
if (zfsvfs->z_os->os_sync == ZFS_SYNC_ALWAYS)
1207-
zil_commit(zilog, 0);
1206+
if (error == 0 && zfsvfs->z_os->os_sync == ZFS_SYNC_ALWAYS)
1207+
error = zil_commit(zilog, 0);
12081208

12091209
zfs_exit(zfsvfs, FTAG);
12101210
return (error);
@@ -1392,14 +1392,15 @@ zfs_mkdir(znode_t *dzp, char *dirname, vattr_t *vap, znode_t **zpp,
13921392

13931393
zfs_dirent_unlock(dl);
13941394

1395-
if (zfsvfs->z_os->os_sync == ZFS_SYNC_ALWAYS)
1396-
zil_commit(zilog, 0);
1397-
13981395
if (error != 0) {
13991396
zrele(zp);
14001397
} else {
14011398
zfs_znode_update_vfs(dzp);
14021399
zfs_znode_update_vfs(zp);
1400+
1401+
if (zfsvfs->z_os->os_sync == ZFS_SYNC_ALWAYS)
1402+
error = zil_commit(zilog, 0);
1403+
14031404
}
14041405
zfs_exit(zfsvfs, FTAG);
14051406
return (error);
@@ -1528,8 +1529,8 @@ zfs_rmdir(znode_t *dzp, char *name, znode_t *cwd, cred_t *cr,
15281529
zfs_znode_update_vfs(zp);
15291530
zrele(zp);
15301531

1531-
if (zfsvfs->z_os->os_sync == ZFS_SYNC_ALWAYS)
1532-
zil_commit(zilog, 0);
1532+
if (error == 0 && zfsvfs->z_os->os_sync == ZFS_SYNC_ALWAYS)
1533+
error = zil_commit(zilog, 0);
15331534

15341535
zfs_exit(zfsvfs, FTAG);
15351536
return (error);
@@ -2630,8 +2631,8 @@ zfs_setattr(znode_t *zp, vattr_t *vap, int flags, cred_t *cr, zidmap_t *mnt_ns)
26302631
}
26312632

26322633
out2:
2633-
if (os->os_sync == ZFS_SYNC_ALWAYS)
2634-
zil_commit(zilog, 0);
2634+
if (err == 0 && os->os_sync == ZFS_SYNC_ALWAYS)
2635+
err = zil_commit(zilog, 0);
26352636

26362637
out3:
26372638
kmem_free(xattr_bulk, sizeof (sa_bulk_attr_t) * bulks);
@@ -3235,8 +3236,8 @@ zfs_rename(znode_t *sdzp, char *snm, znode_t *tdzp, char *tnm,
32353236
zfs_dirent_unlock(sdl);
32363237
zfs_dirent_unlock(tdl);
32373238

3238-
if (zfsvfs->z_os->os_sync == ZFS_SYNC_ALWAYS)
3239-
zil_commit(zilog, 0);
3239+
if (error == 0 && zfsvfs->z_os->os_sync == ZFS_SYNC_ALWAYS)
3240+
error = zil_commit(zilog, 0);
32403241

32413242
zfs_exit(zfsvfs, FTAG);
32423243
return (error);
@@ -3436,7 +3437,7 @@ zfs_symlink(znode_t *dzp, char *name, vattr_t *vap, char *link,
34363437
*zpp = zp;
34373438

34383439
if (zfsvfs->z_os->os_sync == ZFS_SYNC_ALWAYS)
3439-
zil_commit(zilog, 0);
3440+
error = zil_commit(zilog, 0);
34403441
} else {
34413442
zrele(zp);
34423443
}
@@ -3670,18 +3671,20 @@ zfs_link(znode_t *tdzp, znode_t *szp, char *name, cred_t *cr,
36703671

36713672
zfs_dirent_unlock(dl);
36723673

3673-
if (!is_tmpfile && zfsvfs->z_os->os_sync == ZFS_SYNC_ALWAYS)
3674-
zil_commit(zilog, 0);
3675-
3676-
if (is_tmpfile && zfsvfs->z_os->os_sync != ZFS_SYNC_DISABLED) {
3677-
txg_wait_flag_t wait_flags =
3678-
spa_get_failmode(dmu_objset_spa(zfsvfs->z_os)) ==
3679-
ZIO_FAILURE_MODE_CONTINUE ? TXG_WAIT_SUSPEND : 0;
3680-
error = txg_wait_synced_flags(dmu_objset_pool(zfsvfs->z_os),
3681-
txg, wait_flags);
3682-
if (error != 0) {
3683-
ASSERT3U(error, ==, ESHUTDOWN);
3684-
error = SET_ERROR(EIO);
3674+
if (error == 0) {
3675+
if (!is_tmpfile && zfsvfs->z_os->os_sync == ZFS_SYNC_ALWAYS)
3676+
error = zil_commit(zilog, 0);
3677+
3678+
if (is_tmpfile && zfsvfs->z_os->os_sync != ZFS_SYNC_DISABLED) {
3679+
txg_wait_flag_t wait_flags =
3680+
spa_get_failmode(dmu_objset_spa(zfsvfs->z_os)) ==
3681+
ZIO_FAILURE_MODE_CONTINUE ? TXG_WAIT_SUSPEND : 0;
3682+
error = txg_wait_synced_flags(
3683+
dmu_objset_pool(zfsvfs->z_os), txg, wait_flags);
3684+
if (error != 0) {
3685+
ASSERT3U(error, ==, ESHUTDOWN);
3686+
error = SET_ERROR(EIO);
3687+
}
36853688
}
36863689
}
36873690

@@ -3939,8 +3942,13 @@ zfs_putpage(struct inode *ip, struct page *pp, struct writeback_control *wbc,
39393942

39403943
zfs_rangelock_exit(lr);
39413944

3942-
if (need_commit)
3943-
zil_commit(zfsvfs->z_log, zp->z_id);
3945+
if (need_commit) {
3946+
err = zil_commit(zfsvfs->z_log, zp->z_id);
3947+
if (err != 0) {
3948+
zfs_exit(zfsvfs, FTAG);
3949+
return (err);
3950+
}
3951+
}
39443952

39453953
dataset_kstats_update_write_kstats(&zfsvfs->z_kstat, pglen);
39463954

0 commit comments

Comments
 (0)