On 11.07.2011 22:45, Hugo Mills wrote: > OK, here's the remainder of my comments for this file. Not much for > this bit -- just one comment about locking, a reminder, and an > observation.
Again, I ripped out the bits I simply corrected. My comments below. > [...] > >> +static int scrub_write_progress(pthread_mutex_t *m, const char *fsid, >> + struct scrub_progress* data, int n) >> +{ >> + int ret; >> + int fd; >> + int old; >> + >> + ret = pthread_mutex_lock(m); >> + if (ret) { >> + ret = -errno; >> + goto out; >> + } >> + >> + pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &old); > > Probably not massively important, but you don't check for the > return value of this call or its counterpart at the end of this > function. pthread_* return values where wrong throughout the code. Good that you pointed at this one. It's all fixed now. > [...] > >> +static void *scrub_one_dev(void *ctx) >> +{ >> + struct scrub_progress *sp = ctx; >> + int ret; >> + struct timeval tv; >> + >> + sp->stats.canceled = 0; >> + sp->stats.duration = 0; >> + sp->stats.finished = 0; >> + >> + ret = ioctl(sp->fd, BTRFS_IOC_SCRUB, &sp->scrub_args); >> + gettimeofday(&tv, NULL); >> + sp->ret = ret; >> + sp->stats.duration = tv.tv_sec - sp->stats.t_start; >> + sp->stats.canceled = !!ret; >> + sp->ioctl_errno = errno; >> + ret = pthread_mutex_lock(&sp->progress_mutex); >> + if (ret) >> + return ERR_PTR(-errno); >> + sp->stats.finished = 1; >> + ret = pthread_mutex_unlock(&sp->progress_mutex); > > If you downgrade .finished to a plain int, rather than a u64, then > is this locking actually be needed? You have one place where the lock > is held to write a single value (which is atomic for an int, IIRC), > and you have another place where you hold the lock to read and compare > it. I don't see any problem with removing the lock for that. Conclusion first: I want to stick with the mutex. My reasoning: - this is by no means time critical code - the mutexes do the memory barriers required for synchronization - using int instead of u64 would complicate the kvread macros Thanks, -Jan -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html