Make fsck(8) use pread(2)
ClosedPublic
Actions

Authored by trasz on Oct 16 2018, 3:42 PM.

Details

Reviewers

mckusick
kib

Commits

rS342115: Make fsck(8) use pread(2). This cuts the number of syscalls by half.

Summary

Make fsck(8) use pread(2). This cuts the number of syscalls by half.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 21224
Build 20570: arc lint + arc unit

Event Timeline

trasz created this revision.Oct 16 2018, 3:42 PM

Herald added a subscriber: imp. · View Herald TranscriptOct 16 2018, 3:42 PM

Harbormaster completed remote builds in B20243: Diff 49223.Oct 16 2018, 3:42 PM

trasz added a reviewer: mckusick.Oct 16 2018, 3:43 PM

trasz added a reviewer: kib.

This change looks fine to me.

This revision is now accepted and ready to land.Oct 16 2018, 6:51 PM

kib added inline comments.Oct 17 2018, 11:10 AM

sbin/fsck_ffs/fsutil.c
609	Are callers depend on the specific file position after the call ?
614–615	Why did not you converted these uses ?

In answer to kib:

The callers do not depend on the seek offset being set after the call.

The other three places in fsutil.c that do lseek followed by read/write (error followup in blread and blwrite along with blzero) should also be changed to use pread/pwrite.

The reason I hadn't changed the other instances is that while trivial, they are somewhat harder to test, and I've been bitten by trivial changes to rarely excercised branches in the past.

I prefer not to do things halfway. Either we use lseek/read or we use pread. Having it half & half is just crappy coding style. It is possible to write some tests that will exercise the error cases, so if you want to make this change, then I would like you to do it completely.

This revision now requires changes to proceed.Oct 19 2018, 2:39 AM

Use pread/pwrite also in error cases.

Harbormaster completed remote builds in B21224: Diff 51187.Nov 27 2018, 12:26 PM

kib accepted this revision.Nov 27 2018, 2:14 PM

kib added inline comments.

sbin/fsck_ffs/fsutil.c
573	It would be _very_ useful to print errno, in the followup commit.
618	Perhaps trailing ',' should be fixed, in the follow-up commit.

This now looks good to me. I concur with kib's followup comments.

This revision is now accepted and ready to land.Nov 29 2018, 12:33 AM

Closed by commit rS342115: Make fsck(8) use pread(2). This cuts the number of syscalls by half. (authored by trasz). · Explain WhyDec 15 2018, 11:36 AM

This revision was automatically updated to reflect the committed changes.

trasz marked an inline comment as done.Dec 15 2018, 11:46 AM

trasz added inline comments.

sbin/fsck_ffs/fsutil.c
573	Thanks for suggestion; see https://reviews.freebsd.org/D18570. It adds errno reporting to the normal path, in the other 'if' branch.

Revision Contents
Changeset List

Path

Size

sbin/

fsck_ffs/

fsutil.c

19 lines

Diff 51187

View Options

Make fsck(8) use pread(2)ClosedPublicActions