From owner-svn-src-all@freebsd.org Sat Dec 15 17:17:22 2018 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 05C601326AEF; Sat, 15 Dec 2018 17:17:22 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail106.syd.optusnet.com.au (mail106.syd.optusnet.com.au [211.29.132.42]) by mx1.freebsd.org (Postfix) with ESMTP id 34D6780CBF; Sat, 15 Dec 2018 17:17:20 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from [192.168.0.102] (c110-21-101-228.carlnfd1.nsw.optusnet.com.au [110.21.101.228]) by mail106.syd.optusnet.com.au (Postfix) with ESMTPS id 534713CFA91; Sun, 16 Dec 2018 04:17:11 +1100 (AEDT) Date: Sun, 16 Dec 2018 04:17:10 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Edward Tomasz Napierala cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r342115 - head/sbin/fsck_ffs In-Reply-To: <201812151136.wBFBaKV3058236@repo.freebsd.org> Message-ID: <20181216033655.G5660@besplex.bde.org> References: <201812151136.wBFBaKV3058236@repo.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.2 cv=DZtnkrlW c=1 sm=1 tr=0 a=PalzARQSbocsUSjMRkwAPg==:117 a=PalzARQSbocsUSjMRkwAPg==:17 a=kj9zAlcOel0A:10 a=iBufN-CbBZ9qPfiJausA:9 a=04TOIXbChQNhCiql:21 a=-gMqSNPqehXkdgwA:21 a=CjuIK1q_8ugA:10 X-Rspamd-Queue-Id: 34D6780CBF X-Spamd-Bar: ------ Authentication-Results: mx1.freebsd.org X-Spamd-Result: default: False [-7.00 / 15.00]; NEURAL_HAM_MEDIUM(-1.00)[-1.000,0]; NEURAL_HAM_LONG(-1.00)[-0.998,0]; REPLY(-4.00)[]; NEURAL_HAM_SHORT(-1.00)[-0.998,0] X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 15 Dec 2018 17:17:22 -0000 On Sat, 15 Dec 2018, Edward Tomasz Napierala wrote: > Log: > Make fsck(8) use pread(2). This cuts the number of syscalls by half. > > Reviewed by: kib, mckusick This cuts the error handling for reads by a bit more than half. This also makes fsck use pwrite(). Cutting the error handling is almost a feature for writes. > Modified: head/sbin/fsck_ffs/fsutil.c > ============================================================================== > --- head/sbin/fsck_ffs/fsutil.c Sat Dec 15 11:32:11 2018 (r342114) > +++ head/sbin/fsck_ffs/fsutil.c Sat Dec 15 11:36:20 2018 (r342115) > @@ -576,9 +576,7 @@ blread(int fd, char *buf, ufs2_daddr_t blk, long size) > slowio_start(); > totalreads++; > diskreads++; > - if (lseek(fd, offset, 0) < 0) > - rwerror("SEEK BLK", blk); > - else if (read(fd, buf, (int)size) == size) { > + if (pread(fd, buf, (int)size, offset) == size) { > if (bkgrdflag) > slowio_end(); > return (0); Just after here, the comment before the error handling says that the error handling is simpler here because it is just for true read/write errors. > @@ -595,14 +593,11 @@ blread(int fd, char *buf, ufs2_daddr_t blk, long size) > } else > rwerror("READ BLK", blk); > > - if (lseek(fd, offset, 0) < 0) > - rwerror("SEEK BLK", blk); This used to exit in many cases. The seek error is unrecoverable. (Usually the previous lseek() fails before this is reached.) > errs = 0; > memset(buf, 0, (size_t)size); > printf("THE FOLLOWING DISK SECTORS COULD NOT BE READ:"); > for (cp = buf, i = 0; i < size; i += secsize, cp += secsize) { > - if (read(fd, cp, (int)secsize) != secsize) { > - (void)lseek(fd, offset + i + secsize, 0); This used to handle lseek() errors sloppily. > + if (pread(fd, cp, (int)secsize, offset + i) != secsize) { > if (secsize != dev_bsize && dev_bsize != 1) > printf(" %jd (%jd),", > (intmax_t)(blk * dev_bsize + i) / secsize, This handles pread() sloppily. It checks for errors, but proceeds to spew errors for the whole block, just like the sloppy lseek() error handling except now that was protected by 2 instances of non-sloppy lseek() error handling for the start of the block. > @@ -629,22 +624,16 @@ blwrite(int fd, char *buf, ufs2_daddr_t blk, ssize_t s > return; > offset = blk; > offset *= dev_bsize; > - if (lseek(fd, offset, 0) < 0) > - rwerror("SEEK BLK", blk); > - else if (write(fd, buf, size) == size) { > + if (pwrite(fd, buf, size, offset) == size) { > fsmodified = 1; > return; > } > resolved = 0; > rwerror("WRITE BLK", blk); > - if (lseek(fd, offset, 0) < 0) > - rwerror("SEEK BLK", blk); > printf("THE FOLLOWING SECTORS COULD NOT BE WRITTEN:"); > for (cp = buf, i = 0; i < size; i += dev_bsize, cp += dev_bsize) > - if (write(fd, cp, dev_bsize) != dev_bsize) { > - (void)lseek(fd, offset + i + dev_bsize, 0); > + if (pwrite(fd, cp, dev_bsize, offset + i) != dev_bsize) > printf(" %jd,", (intmax_t)blk + i / dev_bsize); > - } > printf("\n"); > return; > } Not so similarly. Everything is the same except now it is a feature to not bail out on the first sign of a problem. Only the error spew is bad here. Of course, lseek errors "can't happen". It is interesting that writes for the error case are done with size dev_bsize, while reads for the error case are done with size secsize. This is very confusing. Both sizes are initially initialized to DEV_BSIZE, which cannot work on disks with large sectors, then bootstrapped confusingly differently by probing for metadata. There is also real_dev_bsize, which suj.c sets to secsize. The printf()s for both claim to access SECTORs. Writes for to attempt to recover from errors by retrying (only once here) should use the smallest size that the hardware supports. This is the sector size unless sectors are virtual. Hopefully dev_bsize == secsize for all writes, since it is too dangerous to write before you know the sector size. Bruce