On Sun, Oct 16, 2016 at 10:19:53AM +0200, Stefan Fritsch wrote:
> On Sun, 16 Oct 2016, Stefan Fritsch wrote:
> > * When a R/W mount is updated to R/O. I will send patches for this in a
> > separate mail.
>
> Part 1: Add an ioctl:
>
>
> add DIOCCACHESYNC
>
> Add an ioctl to tell storage devices to flush their internal caches.
> Ported from netbsd by pedro@
>
> OK?
Some remarks inline
> @@ -848,6 +848,10 @@ wdioctl(dev_t dev, u_long xfer, caddr_t addr, int flag,
> struct proc *p)
> }
> #endif
>
> + /* XXX pedro: should set AT_WAIT according to force flag */
Does this comment help? Either do (*(int *)addr ? AT_WAIT : 0)
or just pass AT_WAIT. But don't force all people reading the code
to think about it.
I think the AT_WAIT code is correct and the comment is wrong. In
sdioctl() the force flag overrides the SDF_DIRTY flag. This is
different from setting AT_WAIT. Just remove the pedro comment.
Then we have the NetBSD code.
> + case DIOCCACHESYNC:
> + return wd_flushcache(wd, AT_WAIT);
> +
All other cases have a "goto exit". With the return we miss a
device_unref().
> default:
> error = wdc_ioctl(wd->drvp, xfer, addr, flag, p);
> goto exit;
> @@ -1067,13 +1071,13 @@ wd_get_params(struct wd_softc *wd, u_int8_t flags,
> struct ataparams *params)
> }
> }
>
> -void
> +int
> wd_flushcache(struct wd_softc *wd, int flags)
> {
> struct wdc_command wdc_c;
>
> if (wd->drvp->ata_vers < 4) /* WDCC_FLUSHCACHE is here since ATA-4 */
> - return;
> + return EIO;
> bzero(&wdc_c, sizeof(struct wdc_command));
> wdc_c.r_command = (wd->sc_flags & WDF_LBA48 ? WDCC_FLUSHCACHE_EXT :
> WDCC_FLUSHCACHE);
> @@ -1088,20 +1092,18 @@ wd_flushcache(struct wd_softc *wd, int flags)
> if (wdc_exec_command(wd->drvp, &wdc_c) != WDC_COMPLETE) {
> printf("%s: flush cache command didn't complete\n",
> wd->sc_dev.dv_xname);
> + return EIO;
> }
> - if (wdc_c.flags & AT_TIMEOU) {
> - printf("%s: flush cache command timeout\n",
> - wd->sc_dev.dv_xname);
> + if (wdc_c.flags & AT_ERROR) {
> + if (wdc_c.r_error == WDCE_ABRT) /* command not supported */
> + return ENODEV;
> }
> - if (wdc_c.flags & AT_DF) {
> - printf("%s: flush cache command: drive fault\n",
> + if (wdc_c.flags & (AT_ERROR | AT_TIMEOU | AT_DF)) {
> + printf("%s: flush cache command timeout\n",
> wd->sc_dev.dv_xname);
NetBSD prints the flags here. If we merge the cases, debugging
will be harder if don't print the relevant bits.
> + return EIO;
> }
> - /*
> - * Ignore error register, it shouldn't report anything else
> - * than COMMAND ABORTED, which means the device doesn't support
> - * flush cache
> - */
> + return 0;
> }
Instead of merging the cases, which was done independently in NetBSD,
I would perfer to take their original commit.
http://cvsweb.netbsd.org/bsdweb.cgi/src/sys/dev/ata/wd.c.diff?r1=1.275&r2=1.276&only_with_tag=MAIN&f=h
Also I think their EIO/ENODEV choices are better.
> @@ -999,6 +999,15 @@ sdioctl(dev_t dev, u_long cmd, caddr_t addr, int flag,
> struct proc *p)
> error = sd_ioctl_cache(sc, cmd, (struct dk_cache *)addr);
> goto exit;
>
> + case DIOCCACHESYNC:
> + if (!ISSET(flag, FWRITE)) {
> + error = EBADF;
> + goto exit;
> + }
> + if ((sc->flags & SDF_DIRTY) != 0 || *(int *)addr != 0)
> + error = sd_flush(sc, 0);
> + return (error);
You need a goto exit to call device_unref().
> +
> default:
> if (part != RAW_PART) {
> error = ENOTTY;
bluhm
