Re: [PATCH v2 2/2] xfstests: introduce 280 for SEEK_DATA/SEEK_HOLE copy check

Jeff Liu
2012-02-07T07:40:46Z

Hi Mark,

On 02/07/2012 06:30 AM, Mark Tinguely wrote:

> On 02/06/12 08:30, Jeff Liu wrote:
>> Introduce 280 for SEEK_DATA/SEEK_HOLE copy check.
>>
>> Signed-off-by: Jie Liu<jeff.liu@oracle.com>
>>
>> ---
>
>>
>> diff --git a/src/seek_copy_tester.c b/src/seek_copy_tester.c
>> new file mode 100644
>> index 0000000..ddf683f
>> --- /dev/null
>> +++ b/src/seek_copy_tester.c
>
>> +static size_t
>> +full_write(int fd, const void *buf, size_t count)
>> +{
>> + size_t total = 0;
>> + const char *ptr = (const char *) buf;
>> +
>> + while (count> 0) {
>> + ssize_t n = write(fd, ptr, count);
>> + if (n< 0) {
>> + if (errno == EINTR)
>> + continue;
>
> Wouldn't you want to stop the write loop if interrupted?

As this routine was called "full_write" which means it is expect to write as much as it can.
So I would keep retrying if the process was interrupted by EINTR.
Would you please give some opinions whether this approach is not suitable in this circumstance?

>
>> + error("full_write() failed as %s", strerror(errno));
>> + break;
>> + }
>> +
>> + if (n == 0) {
>> + error("full_write() ZERO bytes transferred");
>
> You can tell them how many (total) bytes were written before the error.
> Same in full_pwrite()

This really a good idea!

>
>> +create_data_and_holes(int fd, size_t nr_total_bytes, off_t start_offset,
>> + uint64_t nr_skip_bytes, uint64_t nr_data_bytes,
>> + int wrote_hole_at_eof)
>> +{
>> + int ret = 0;
>> + off_t total = nr_total_bytes;
>> + off_t data_len = nr_data_bytes;
>> + off_t off = start_offset;
>> + char buf[BUF_SIZE];
>> +
>> + memset(buf, 'A', sizeof(buf));
>> +

>> + total -= start_offset;

>> + while (total> 0) {
>
> + data_len = nr_data_bytes; /* see below */
>
>> + do {
>> + size_t nr_write = MIN(total, BUF_SIZE);
>
> There are 2 dependencies:
> 1) The last nr_data_bytes does not exceed total (outer loop)
> 2) The remaining data size (data_len) could be less than BUF_SIZE
> (if data_len is not a multiple of BUF_SIZE for example).
>
>> + if (full_pwrite(fd, buf, nr_write, off) != nr_write) {
>> + error("full_pwrite() failed as %s", strerror(errno));
>> + ret = -1;
>> + goto out;
>> + }
>> +
>> + off += nr_write;
>> + data_len -= nr_write;
>> + } while (data_len> 0);
>
> data_len is <= 0 so the inner loop will execute only once if you have
> more than one chunk of data.

Thanks for the comments!

So I'll revise the code logic as following:
1) Ensure "total > start_offset" before go into this routine, i.e, start offset should less than the total file size.
2) The left total should larger than data_len(nr_data_bytes).
3) Calculate the nr_write based on above two points.

Thanks,
-Jeff

>> +
>> + off += (nr_skip_bytes + nr_data_bytes);
>> + total -= off;
>> + }
>> +
>> + if (off< nr_total_bytes) {
>> + if (wrote_hole_at_eof) {
>> + ret = ftruncate(fd, nr_total_bytes);
>> + if (ret< 0) {
>> + error("truncate source file to %lld bytes failed as %s",
>> + (long long)nr_total_bytes, strerror(errno));
>> + }
>> + }
>> +
>> + ret = write_zeros(fd, nr_total_bytes - off);
>> + if (ret< 0) {
>> + error("write_zeros to end of file failed as %s",
>> + strerror(errno));
>> + }
>> + }
>> +
>> +out:
>> + return ret;
>> +}
>
> It is shaping up nicely.
>
> --Mark