xfs
[Top] [All Lists]

Re: [PATCH v2] xfstests: pwrite hang when writing from mmaped buffer of

To: "Zhong, Xin" <xin.zhong@xxxxxxxxx>
Subject: Re: [PATCH v2] xfstests: pwrite hang when writing from mmaped buffer of the same page
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Tue, 14 Dec 2010 15:42:59 +1100
Cc: xfs@xxxxxxxxxxx
In-reply-to: <1292295652-9508-1-git-send-email-xin.zhong@xxxxxxxxx>
References: <1292295652-9508-1-git-send-email-xin.zhong@xxxxxxxxx>
User-agent: Mutt/1.5.20 (2009-06-14)
On Tue, Dec 14, 2010 at 11:00:52AM +0800, Zhong, Xin wrote:
> The problem is found in meego testing on btrfs:
> http://bugs.meego.com/show_bug.cgi?id=6672
> 
> Reviewed-by: Hellwig, Christoph <hch@xxxxxxxxxxxxx>
> Signed-off-by: Zhong, Xin <xin.zhong@xxxxxxxxx>
> ---
>  248                       |   58 +++++++++++++++++++++++++++++++++
>  248.out                   |    2 +
>  group                     |    1 +
>  src/Makefile              |    2 +-
>  src/pwrite_mmap_blocked.c |   78 
> +++++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 140 insertions(+), 1 deletions(-)
>  create mode 100755 248
>  create mode 100644 248.out
>  create mode 100644 src/pwrite_mmap_blocked.c
> 
> diff --git a/248 b/248
> new file mode 100755
> index 0000000..a4664d9
> --- /dev/null
> +++ b/248
> @@ -0,0 +1,58 @@
> +#! /bin/bash
> +# FS QA Test No. 248
> +#
> +# Test for pwrite hang problem when writing from mmaped buffer of the same 
> page 
> +#
> +#-----------------------------------------------------------------------
> +# Copyright (c) 2010 YOUR NAME HERE.  All Rights Reserved.
                        ^^^^^^^^^^^^^^

Needs updating, I think. ;)

While I'm here, a two minute review...

....

> +_cleanup()
> +{
> +    cd /
> +    rm -f $tmp.*
> +}
> +
> +# get standard environment, filters and checks
> +. ./common.rc
> +. ./common.filter
> +
> +# real QA test starts here
> +
> +# Modify as appropriate.
> +_supported_fs generic
> +_supported_os Linux
> +
> +TESTFILE=$TEST_DIR/test_file
> +TEST_PROG=$here/src/pwrite_mmap_blocked
> +
> +$TEST_PROG $TESTFILE
> +rm -f $TESTFILE

This rm should be in the _cleanup() function so that it is removed
even when the test is aborted.

.....

> diff --git a/group b/group
> index 0f94dd9..ddda26c 100644
> --- a/group
> +++ b/group
> @@ -361,3 +361,4 @@ deprecated
>  245 auto quick dir
>  246 auto quick rw
>  247 auto quick rw
> +248 other quick rw 

Why not the auto group?

> +char *progname;
> +loff_t size;
> +int fd;
> +void *mapped_mem;

None of these need to be global.

> +
> +
> +int main(int argc, char *argv[])
> +{
> +     int ret;
> +
> +     progname = argv[0];
> +     size = 5;
> +     fd = open(argv[1], O_RDWR|O_TRUNC|O_CREAT, 0666);
> +     if (fd < 0) {
> +             fprintf(stderr, "%s: Cannot open `%s': %s\n",
> +                     progname, argv[1], strerror(errno));
> +             exit(1);
> +     }
> +
> +     char *cc = "01234";

Don't mix declarations in the the code. Better would be:

        int ret;
        char *cc = "01234";

        progname = argv[0];
        size = strlen(cc);
        ....


> +     if ((ret = pwrite(fd, (const char *)cc,
> +                             size, 0)) != size) {

        ret = pwrite(fd, cc, size, 0);
        if (ret != size) {

> +             fprintf(stderr, "%s: pwrite returned %d\n",
> +                     __FUNCTION__, ret);
> +             perror("pwrite");
> +             exit(1);
> +     }

No need for both fprintf and perror calls.

> +
> +     mapped_mem = mmap(0, size, PROT_READ|PROT_WRITE, MAP_SHARED, fd, 0);
> +     if (mapped_mem == MAP_FAILED) {
> +             perror("mmap");
> +             exit(1);
> +     }
> +     loff_t amount = 1;
> +     loff_t from = 2;
> +     loff_t to = 3;
> +     printf("pwrite %Ld bytes from %Ld to %Ld\n", amount, from, to);
> +     if ((ret = pwrite(fd, (char *)mapped_mem + from, amount, to)) != 
> amount) {

        ret = pwrite(fd, (char *)mapped_mem + from, amount, to));
        if (ret != amount) {

> +             printf("%s: pwrite returned %d, not %Ld\n",
> +                                     __FUNCTION__, ret, amount);
> +             if (errno == EFAULT) {
> +                     printf("pwrite: EFAULT\n");
> +             } else if (ret < 0) {
> +                     perror("pwrite");
> +                     exit(1);
> +             }

A single call to perror() will be sufficient here...

> +     }
> +     munmap(mapped_mem,0);
> +     close(fd);
> +     exit(0);
> +}

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

<Prev in Thread] Current Thread [Next in Thread>