daily CVS update output
Updating src tree: P src/lib/libcurses/setterm.c P src/sys/dev/hyperv/hvs.c P src/tests/lib/libcurses/director/director.c Updating xsrc tree: Killing core files: Updating file list: -rw-rw-r-- 1 srcmastr netbsd 41350317 Jun 11 03:03 ls-lRA.gz
Re: st.c update has broken dump multi-tape support
On Thu, Jun 10, 2021 at 11:10:23AM +0200, Frank Kardel wrote: > > I meant the section in ststart1 where error is set to zero followed by goto > out inf the fixed blocksize part. > Sorry, yes you are correct. I ignored that because I am using variable blocks...oops. -- Brett Lymn -- Sent from my NetBSD device. "We are were wolves", "You mean werewolves?", "No we were wolves, now we are something else entirely", "Oh"
Re: st.c update has broken dump multi-tape support
On Thu, Jun 10, 2021 at 05:38:34PM +0200, Michael van Elst wrote: > > Sorry, it doesn't fix the EOM handling, just the biodone. > mea culpa... I should take more time before replying... > I still have to understand the EOM logic :) > I will post up a diff later that appears to work for me. From what the code used to do and the description Frank posted EOM is indicated by a 0 length write with no error iff the early warning flag is set. I haven't checked but I ASSuME that ST_EOM_PENDING will only be set if the early warning flag is on. -- Brett Lymn -- Sent from my NetBSD device. "We are were wolves", "You mean werewolves?", "No we were wolves, now we are something else entirely", "Oh"
Re: st.c update has broken dump multi-tape support
On Thu, Jun 10, 2021 at 04:57:21PM +0200, Frank Kardel wrote: > Hi ! > > I assumed Michael was proposing a solution for the missing biodone() in the > fixed block path (though that part was missing in the patch). Yes, biodone needs to be called without the lock being held, so not in ststart1(). Greetings, -- Michael van Elst Internet: mlel...@serpens.de "A potential Snark may lurk in every tree."
Re: st.c update has broken dump multi-tape support
On Thu, Jun 10, 2021 at 11:22:45PM +0930, Brett Lymn wrote: > On Thu, Jun 10, 2021 at 12:13:22PM +0200, Michael van Elst wrote: > > On Thu, Jun 10, 2021 at 12:02:19PM +0200, Michael van Elst wrote: > > > > > If you don't like the fake errno, the function needs to return > > > two values, the error value and a boolean to finish the > > > unqueued request. Cleaner, but more changes. > > > > E.g. (not even compile-tested): > > > > I don't think that is quite right. Sorry, it doesn't fix the EOM handling, just the biodone. I still have to understand the EOM logic :) -- Michael van Elst Internet: mlel...@serpens.de "A potential Snark may lurk in every tree."
Re: st.c update has broken dump multi-tape support
Hi !
I assumed Michael was proposing a solution for the missing biodone() in
the fixed block path (though that part was missing in the patch).
We should try to fix both issues (write return code and missing biodone)
with hopefully minimal changes without sacrificing clarity and abstraction.
IMHO ststart() should manage the interface to ststart1() but not look
into specific bits (ST_EOM_PENDING) and ststart1() should signal
ststart() errno and biodone(). Thus I did see
merit in Michael's proposal. This is a style discussion, however.
On a more important note: Looking into the code again we also seem to
miss clearing ST_{EOM,EIO}_PENDING that is something that was present in
1.231. Clearing that would get st.c
in-line again with st(4).
Frank
On 06/10/21 15:52, Brett Lymn wrote:
On Thu, Jun 10, 2021 at 12:13:22PM +0200, Michael van Elst wrote:
On Thu, Jun 10, 2021 at 12:02:19PM +0200, Michael van Elst wrote:
If you don't like the fake errno, the function needs to return
two values, the error value and a boolean to finish the
unqueued request. Cleaner, but more changes.
E.g. (not even compile-tested):
I don't think that is quite right. At line 1204 error is set to EIO, even with
your changes
b_error will still get set to EIO when EOM_PENDING is true. Previously b_error
was only set
b_error would be set to EIO in previous versions this would only happen if
there was no
ST_EOM_PENDING flag set. I did a much smaller change in ststart inside
the if at line 1290 I added a check to only set b_error to the value of error
unless error
==EIO and st->flags contains ST_EOM_PENDING. This change made dump perform as
expected and
prompt for a new tape.
Re: st.c update has broken dump multi-tape support
On Thu, Jun 10, 2021 at 12:13:22PM +0200, Michael van Elst wrote: > On Thu, Jun 10, 2021 at 12:02:19PM +0200, Michael van Elst wrote: > > > If you don't like the fake errno, the function needs to return > > two values, the error value and a boolean to finish the > > unqueued request. Cleaner, but more changes. > > E.g. (not even compile-tested): > I don't think that is quite right. At line 1204 error is set to EIO, even with your changes b_error will still get set to EIO when EOM_PENDING is true. Previously b_error was only set b_error would be set to EIO in previous versions this would only happen if there was no ST_EOM_PENDING flag set. I did a much smaller change in ststart inside the if at line 1290 I added a check to only set b_error to the value of error unless error ==EIO and st->flags contains ST_EOM_PENDING. This change made dump perform as expected and prompt for a new tape. -- Brett Lymn -- Sent from my NetBSD device. "We are were wolves", "You mean werewolves?", "No we were wolves, now we are something else entirely", "Oh"
Re: st.c update has broken dump multi-tape support
On Thu, Jun 10, 2021 at 12:02:19PM +0200, Michael van Elst wrote:
> If you don't like the fake errno, the function needs to return
> two values, the error value and a boolean to finish the
> unqueued request. Cleaner, but more changes.
E.g. (not even compile-tested):
Index: st.c
===
RCS file: /cvsroot/src/sys/dev/scsipi/st.c,v
retrieving revision 1.240
diff -p -u -r1.240 st.c
--- st.c27 Dec 2019 09:41:51 - 1.240
+++ st.c10 Jun 2021 10:11:47 -
@@ -343,7 +343,7 @@ static int st_mount_tape(dev_t, int);
static voidst_unmount(struct st_softc *, boolean);
static int st_decide_mode(struct st_softc *, boolean);
static voidststart(struct scsipi_periph *);
-static int ststart1(struct scsipi_periph *, struct buf *);
+static int ststart1(struct scsipi_periph *, struct buf *, int *);
static voidstrestart(void *);
static voidstdone(struct scsipi_xfer *, int);
static int st_read(struct st_softc *, char *, int, int);
@@ -1183,13 +1183,13 @@ abort:
* ststart() is called with channel lock held
*/
static int
-ststart1(struct scsipi_periph *periph, struct buf *bp)
+ststart1(struct scsipi_periph *periph, struct buf *bp, int *errnop)
{
struct st_softc *st = device_private(periph->periph_dev);
struct scsipi_channel *chan = periph->periph_channel;
struct scsi_rw_tape cmd;
struct scsipi_xfer *xs;
- int flags, error;
+ int flags, error, complete = 1;
SC_DEBUG(periph, SCSIPI_DB2, ("ststart1 "));
@@ -1299,11 +1299,14 @@ ststart1(struct scsipi_periph *periph, s
error = scsipi_execute_xs(xs);
/* with a scsipi_xfer preallocated, scsipi_command can't fail */
KASSERT(error == 0);
+ if (error == 0)
+ complete = 0;
out:
mutex_exit(chan_mtx(chan));
- return error;
+ *errnop = error;
+ return complete;
}
static void
@@ -1312,7 +1315,7 @@ ststart(struct scsipi_periph *periph)
struct st_softc *st = device_private(periph->periph_dev);
struct scsipi_channel *chan = periph->periph_channel;
struct buf *bp;
- int error;
+ int error, complete;
SC_DEBUG(periph, SCSIPI_DB2, ("ststart "));
@@ -1325,19 +1328,20 @@ ststart(struct scsipi_periph *periph)
iostat_busy(st->stats);
mutex_exit(&st->sc_iolock);
- error = ststart1(periph, bp);
+ complete = ststart1(periph, bp, &error);
mutex_enter(&st->sc_iolock);
- if (error != 0)
+ if (complete) {
iostat_unbusy(st->stats, 0,
((bp->b_flags & B_READ) == B_READ));
- if (error == EAGAIN) {
- bufq_put(st->buf_defer, bp);
- break;
+ if (error == EAGAIN) {
+ bufq_put(st->buf_defer, bp);
+ break;
+ }
}
mutex_exit(&st->sc_iolock);
- if (error != 0) {
+ if (complete) {
bp->b_error = error;
bp->b_resid = bp->b_bcount;
biodone(bp);
--
Michael van Elst
Internet: mlel...@serpens.de
"A potential Snark may lurk in every tree."
Re: st.c update has broken dump multi-tape support
On Thu, Jun 10, 2021 at 11:10:23AM +0200, Frank Kardel wrote: > Hi Brett, > > I meant the section in ststart1 where error is set to zero followed by goto > out inf the fixed blocksize part. The biodone is missing, but also other parts. We have 5 cases. - I/O request is queued, error == 0. -> will be finished in callback. - I/O request is queued, error != 0. -> ststart calls biodone. - I/O request is not queued, error == EAGAIN -> ststart requeues request - I/O request is not queued, error != 0 -> ststart calls biodone. and - I/O request is not queued, error == 0 -> this is broken. I would make the last case return error == -1 instead (!= any possible errno value). In ststart errno is checked != 0, so it will - finish the I/O request for iostat. - call biodone The latter needs an adjustment like: bp->b_error = error < 0 ? 0 : error; so that the fake errno is replaced with end-of-file. If you don't like the fake errno, the function needs to return two values, the error value and a boolean to finish the unqueued request. Cleaner, but more changes. Greetings, -- Michael van Elst Internet: mlel...@serpens.de "A potential Snark may lurk in every tree."
Re: st.c update has broken dump multi-tape support
Hi Brett,
I meant the section in ststart1 where error is set to zero followed by
goto out inf the fixed blocksize part.
on that path the biodone() would be missing - just something I noticed
when looking at the code.
/*
* only FIXEDBLOCK devices have pending I/O or space operations.
*/
if (st->flags & ST_FIXEDBLOCKS) {
/*
* If we are at a filemark but have not reported it yet
* then we should report it now
*/
if (st->flags & ST_AT_FILEMARK) {
if ((bp->b_flags & B_READ) == B_WRITE) {
/*
* Handling of ST_AT_FILEMARK in
* st_space will fill in the right file
* mark count.
* Back up over filemark
*/
if (st_space(st, 0, SP_FILEMARKS, 0)) {
error = EIO;
goto out;
}
} else {
bp->b_resid = bp->b_bcount;
error = 0;
st->flags &= ~ST_AT_FILEMARK;
>>/* XXX missing a biodone() here? */
goto out;
}
}
}
Frank
On 06/10/21 08:42, Brett Lymn wrote:
Hi Frank,
On Thu, Jun 10, 2021 at 07:45:25AM +0200, Frank Kardel wrote:
Could you check whether my suspicion that biodone() may be missing the
ststart1 function in the
It is missing there but is called in ststart if the error is != 0 after
the ststart1 call. I was going to update the ststart function to do
something very close to what you have done.
I have not tested the patch as my machine with the tapes is remote and has
no remote console
and I don't want to brick that while being off-site.
That's ok - I can test here without harming anything, the machine the
tape drive is attached to has to be booted to windows for $WORK during
the day so my testing window is limited :)
Re: st.c update has broken dump multi-tape support
Hi Frank, On Thu, Jun 10, 2021 at 07:45:25AM +0200, Frank Kardel wrote: > > Could you check whether my suspicion that biodone() may be missing the > ststart1 function in the > It is missing there but is called in ststart if the error is != 0 after the ststart1 call. I was going to update the ststart function to do something very close to what you have done. > I have not tested the patch as my machine with the tapes is remote and has > no remote console > > and I don't want to brick that while being off-site. > That's ok - I can test here without harming anything, the machine the tape drive is attached to has to be booted to windows for $WORK during the day so my testing window is limited :) -- Brett Lymn -- Sent from my NetBSD device. "We are were wolves", "You mean werewolves?", "No we were wolves, now we are something else entirely", "Oh"
