On 12/07/2015 02:26 PM, Christoph Hellwig wrote:
>> static __be32
>> +nfsd4_verify_copy(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>> + struct nfsd4_copy *copy, struct file **src, struct file **dst)
>> +{
>> + __be32 status;
>> +
>> + status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->save_fh,
>> + ©->cp_src_stateid, RD_STATE,
>> + src, NULL);
>> + if (status) {
>> + dprintk("NFSD: nfsd4_copy: couldn't process src stateid!\n");
>> + return status;
>> + }
>> +
>> + status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->current_fh,
>> + ©->cp_dst_stateid, WR_STATE,
>> + dst, NULL);
>> + if (status) {
>> + dprintk("NFSD: nfsd4_copy: couldn't process dst stateid!\n");
>> + fput(*src);
>> + }
>
> This is missing a return status. On the clone side that caused really
> hard to debug crashes when xfstests hit this case. While you're at it
> I'd suggest to also kill the nfsd4_verify_copy heper. You might also
> need a check for invalid file types that maps to the correct NFS error
> code, similar to clone.
I just updated against the clone code, and I made some of these changes earlier this afternoon. I kept the verify_copy() helper around and changed clone to call it, since all of the stateid verification code would be almost identical.
>
>> + if (bytes < 0)
>> + status = nfserrno(bytes);
>
> maybe use a goto here?
Maybe. I'll see how the code looks!
>
>> + else {
>> + copy->cp_res.wr_bytes_written = bytes;
>> + copy->cp_res.wr_stable_how = NFS_FILE_SYNC;
>> + copy->cp_consecutive = 1;
>
> is there anything in the linux semantics that guarantees consecutive
> operation?
I think so. The splice fallback iterates starting at the beginning of the file, so if something goes wrong later on then the earlier pages should at least be copied.
>
>> + u64 count)
>> +{
>> + ssize_t bytes;
>> + u64 limit = 0x10000000;
>> +
>> + if (count > limit)
>> + count = limit;
>> +
>> + bytes = vfs_copy_file_range(src, src_pos, dst, dst_pos, count, 0);
>> + if (bytes > 0)
>> + vfs_fsync_range(dst, dst_pos, dst_pos + bytes, 0);
>> + return bytes;
>
> How about returning NFS_UNSTABLE above and avoiding the fsync here?
I was just looking into this, too. I'm trying to figure out the right way to handle this on the client side, since right now we ignore this value. I have gotten as far as "if the file is open with O_SYNC, then we should commit after copying."
Anna
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
